Conversation
|
See also #426 |
I will add that to the Tutorial update project. I don't want to do that update with this PR to maintain some formal scope. I can get another PR in to address that issue. |
gonuke
left a comment
There was a problem hiding this comment.
Thanks for this careful review @abachma2 . I have reviewed all your changes, but haven't done a full read to confirm that the changes are any more/less consistent with the tutorial - I'll trust you on that. There are a few places where I did look at the context of the changes and asked some questions.
source/user/tutorial/add_arche.rst
Outdated
| </archetypes> | ||
|
|
||
| Once complete, append the archetypes section under the control section of input file [#f1]_. | ||
| The order of the archetypes in this block is of minor consequence. Once complete, append the archetypes section under the control section of input file [#f1]_. |
There was a problem hiding this comment.
I'm not sure I know what "minor consequence" means.... does the order matter at all?
There was a problem hiding this comment.
As far as I know, the only ordering that matters in a Cyclus input file is that the control block needs to go first. I don't know if there are any other things that matter. I can change minor to no if that is more accurate.
There was a problem hiding this comment.
I think "no consequence" would be better
There was a problem hiding this comment.
Forgot to save that with my last round of edits. It is now changed.
| There can be multiple ``entry`` blocks within the same institution, hence the | ||
| ``...`` in the block. Multiple entry blocks means that multiple prototypes | ||
| are deployed at the start of the simulation. |
There was a problem hiding this comment.
We don't have ... in this example. I think we need to add that for this to make sense
There was a problem hiding this comment.
I did add that in. Line 193.
| <item> | ||
| <comp>_______</comp> | ||
| <eff>_______</eff> | ||
| <comp>[nuclide]</comp> |
There was a problem hiding this comment.
I think this needs to refer to an element??? or is that just a convention. Separations acts on elements not isotopes...
There was a problem hiding this comment.
Elsewhere on the website I see that we have nuclide: https://fuelcycle.org/user/cycamoreagents.html#cycamore-separations. I don't actually know that answer.
There was a problem hiding this comment.
On a community call, I believe we came to a consensus that this input can be a nuclide because an input of 94000 works in an input file.
| * ``RegionArchetype``: name of the Region archetype you wish to use in your simulation, and the dotted line in this section represents any inputs that the archetype might have. | ||
|
|
There was a problem hiding this comment.
Perhaps a note that RegionArchetype isn't (currently, at least) a valid input in this block and that it must be replaced with the actual region name with an example like <NullRegion>. What you have implies this, but users new to Cyclus/XML may get tripped up by this.
| </facility> | ||
|
|
||
| Append this prototype right after the ``1178MWe BRAIDWOOD_1`` prototype. | ||
| Append this prototype right after the ``1178MWe BRAIDWOOD_1`` prototype in your input file. |
There was a problem hiding this comment.
I think in the last tutorial you changed 1178MWe BRAIDWOOD_1 to 1178MWe ReactorPlant Unit 1
There was a problem hiding this comment.
Yes. I changed it in the one file as a suggestion. If folks think it's a good change, then I'll propagate it throughout the rest of the tutorial.
| </entry> | ||
|
|
||
| below the ``1178MWe BRAIDWOOD_1`` entry block. The Reactor section | ||
| below the ``1178MWe BRAIDWOOD_1`` entry block. The second institution section |
There was a problem hiding this comment.
See above about 1178MWe ReactorPlant Unit 1 vs BRAIDWOOD
dean-krueger
left a comment
There was a problem hiding this comment.
Awesome work, this is a lot better for having been gone through like you did. I left two or three comments separately, but they're all pretty minor. Otherwise looks good to me!
abachma2
left a comment
There was a problem hiding this comment.
Thanks for the reviews @gonuke and @dean-krueger. If you like some of the changes I've made, let me know and I can propagate them through. There are still 1-2 discussion points left.
| .. code-block:: XML | ||
|
|
||
| <outrecipe>[outrecipe]</outrecipe> | ||
| <outrecipe>[string]</outrecipe> |
There was a problem hiding this comment.
I can make that update.
| <config> | ||
| <Archetype> | ||
| <outcommod>outcommod</outcommod> | ||
| <Source> |
There was a problem hiding this comment.
Which one do you think is better for the tutorial? I think putting the archetype name in the <Archetype> area for the template is better, because it's easier to distinguish what are the archetype-specific inputs vs what are the facility inputs (e.g., lifetime, config).
| * ``RegionArchetype``: name of the Region archetype you wish to use in your simulation, and the dotted line in this section represents any inputs that the archetype might have. | ||
|
|
| </facility> | ||
|
|
||
| Append this prototype right after the ``1178MWe BRAIDWOOD_1`` prototype. | ||
| Append this prototype right after the ``1178MWe BRAIDWOOD_1`` prototype in your input file. |
There was a problem hiding this comment.
Yes. I changed it in the one file as a suggestion. If folks think it's a good change, then I'll propagate it throughout the rest of the tutorial.
| </entry> | ||
|
|
||
| below the ``1178MWe BRAIDWOOD_1`` entry block. The Reactor section | ||
| below the ``1178MWe BRAIDWOOD_1`` entry block. The second institution section |
I do! |
source/user/tutorial/add_proto.rst
Outdated
| @@ -488,8 +490,8 @@ The sink facility archetype is: | |||
| <config> | |||
| <Archetype> | |||
There was a problem hiding this comment.
The <Archetype> tag in this template is closed with </Sink>.
abachma2
left a comment
There was a problem hiding this comment.
Thanks for the comments, everyone. I have finished propagating some of the suggested changes through the rest of the tutorial. If there is nothing else that needs to be done to address the matters in #427, this should be ready to merge.
I have also fixed some minor formatting things within the most recent changes.
ConnorWelch-OSU
left a comment
There was a problem hiding this comment.
All of these changes look good to me. I did however notice a typo with the naming of the second reactor, and I called it out in comment on each file that it appears in. I believe I found all of them, but I may have missed some.
|
Thanks for catching all of that @ConnorWelch-OSU. I think we should be good to merge this and start working on some of the other items in #427 and other issues. |
I should have pointed it out explicitly because its kind of a sneaky typo, but the other issue with the second reactor naming is that a lot of them are 1000 Watt electric instead of 1000 Megawatt electric. Sorry about that. |
ConnorWelch-OSU
left a comment
There was a problem hiding this comment.
Everything looks good to me. This should be ready to merge.
gonuke
left a comment
There was a problem hiding this comment.
I think this is ready to merge... there may be additional things we want to change, but I think this large PR has tackled most of it and additional things will emerge better after this has been merged.
|
I believe my concerns were addressed. I'm happy for this to be merged as well! |
Summary of Changes
This PR addresses some of the items identified by me in #427 to update the user tutorial. The changes are generally pretty minor: fixing typos, correcting formats, making things consistent across the tutorial. Some of the wording changes improve the clarity of the tutorial for me and I welcome discussion if you think the changes make things less clear.
Design Notes
Check the box if your change does not break any of the following:
Related CEPs and Issues
This PR addresses some of the items in #427, but not all of them so the issue should remain open after merging.
Associated Developers
Testing and Validation
Reviewers, please refer to the Cyclus Guide for Reviewers.