-
Notifications
You must be signed in to change notification settings - Fork 32
Reducing the verbosity of the tutorial #444
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: source
Are you sure you want to change the base?
Conversation
abachma2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this @ConnorWelch-OSU. I think this PR accomplishes the goal of removing some verbosity from the tutorial.
In the add_arche, add_commod_recipe and sim_parm files, it looks like you took out the completed blocks for users to compare against, but kept those in add_proto and add_reg_inst. Any reason for that inconsistency? I think having the completed blocks in the tutorial before the final input file has value.
| **Note**: There are two blank lines between the end of the control section and | ||
| end of the simulation section. This section of the simulation block will hold | ||
| the rest of the simulation parameter blocks (commodities, facilities, regions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this if we don't have the completed control block anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking in leaving it was that the template that appears earlier in the page does contain the two blank lines. It might make more sense to have it further up the page in the section that describes the different parts of the template. I can make that change if its what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add in those sections. I think they are valuable and make things more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. I have added them in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the added completed control block is good for the beginner as this page is their first introduction to seeing completed cyclus input!
I agree that having completed blocks has value. My opinion is that the completed block should appear in a "Check" section at the end of each page, and that's how they are in |
abachma2
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making that change @ConnorWelch-OSU. I am approving this PR. I'll leave this open for another few days, in case anyone else wants to look at this. Feel free to ping me to merge this if you think enough time has passed and I haven't merged this yet.
meg-krieg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of my comments are cosmetic except for 1 typo.
I do still think the Add Region and Institution activity could be even less verbose. There are a lot of sequences of xml blocks to get to the general template... I am not sure how that reduction would look though.
| </region> | ||
| Let's build the ``ReactorUtility`` institution. This institution has one ``1178MWe ReactorPlant Unit 1`` prototype. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this redundant compared to the above lines still under Activity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make changes improve the wording.
| <lib>lib6</lib> | ||
| <name>arch6</name> | ||
| </spec> | ||
| <spec> | ||
| <lib>lib7</lib> | ||
| <name>arch7</name> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these be lib5/arch5 and lib6/arch6?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I will make those changes.
| **Note**: There are two blank lines between the end of the control section and | ||
| end of the simulation section. This section of the simulation block will hold | ||
| the rest of the simulation parameter blocks (commodities, facilities, regions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the added completed control block is good for the beginner as this page is their first introduction to seeing completed cyclus input!
| </facility> | ||
| .. rubric:: Footnotes | ||
| .. [#f1] The exact order of the sections in a |Cyclus| input file are of minor consequence. The ``control`` sequence must go first, but the other sequences can go in any order that makes sense to the user. The traditional organization of an input file is: control, archetypes, commodities, facilities, regions/institutions, and recipes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this comment isn't for this PR, but is there a better home for this footnote (and the other repeats of this footnote)? Would go with the "reduce" verbosity goal of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that there would be value in stating the information from the footnote early on in the tutorial, but I decided to leave it for this PR so it can be discussed as a separate issue.
typo fix Co-authored-by: meg-krieg <kriegm@oregonstate.edu>
Summary of Changes
This PR address the verbosity of the tutorial identified in #442. To reduce verbosity, repeated examples of XML blocks were removed, leaving only a template for each section of an input file. Additionally, minor changes to the text around where XML blocks were removed has been edited to fit the new context that it exists in.
Design Notes
I did not remove the XML blocks that users can use to check that their input files are correct that are at the end of some pages. There is a discussion to be had on whether these are something that we want to keep or not.
Check the box if your change does not break any of the following:
Related CEPs and Issues
Fixes #442
Associated Developers
Testing and Validation
Reviewers, please refer to the Cyclus Guide for Reviewers.