Skip to content
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

Tutorial #293

Merged
merged 114 commits into from
Feb 25, 2021
Merged

Tutorial #293

merged 114 commits into from
Feb 25, 2021

Conversation

gtw2
Copy link
Contributor

@gtw2 gtw2 commented Jun 28, 2019

Updating the Cyclus user tutorial - Compilation of changes from multiple people at ARFC. Also includes random fixes to the website.

@katyhuff katyhuff self-requested a review January 13, 2021 17:25
Tutorial changes based on comments in Cyclus PR
@katyhuff
Copy link
Member

katyhuff commented Feb 4, 2021

@abachma2 What's the status here?

@abachma2
Copy link
Member

abachma2 commented Feb 4, 2021

I think I have addressed all of the comments from @katyhuff and @nuclearkatie (other than maybe the S vs M for energy state in the ZAID). I know @gonuke had some comments/changes earlier, not sure if he wanted to look at this before it gets merged. I don't know if other people (like @bam241) wanted to look over this before it gets merged.

I haven't made any changes to the data analysis part of the tutorial (source/user/tutorial/data_explorer.rst) because it has been removed from the tutorial right now and I included a reference to cymetric and the examples in that repository. I didn't want to remove the file altogether, because eventually I would like to re-write that page to use cymetric (it currently uses an analysis.py script) and include it back in the tutorial.

Overall, I think this is 95% done, with the last 5% for this PR to come from any further reviews. If no one else wants to add a review then the energy state in the ZAID needs to be resolved then this can be merged.

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a first round of review. I want to get these comments to you now since I'm not sure when I'll get to the rest....

source/user/tutorial/sim_parm.rst Outdated Show resolved Hide resolved
source/user/tutorial/sim_parm.rst Outdated Show resolved Hide resolved
source/user/tutorial/sim_parm.rst Outdated Show resolved Hide resolved
</simulation>

The lifetime of a |Cyclus| simulation is determined by its
**duration**, the number of months |Cyclus| will
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we use a fixed font format for keywords?

Suggested change
**duration**, the number of months |Cyclus| will
`duration`, the number of months |Cyclus| will

source/user/tutorial/sim_parm.rst Outdated Show resolved Hide resolved
source/user/tutorial/add_reg_inst.rst Outdated Show resolved Hide resolved
source/user/tutorial/add_reg_inst.rst Outdated Show resolved Hide resolved
source/user/tutorial/run_cyclus_native.rst Outdated Show resolved Hide resolved
Simple simulations can easily be expanded into more complex problems. To demonstrate this,
we will now add a second reactor, ``1000We Lightwater-1``, to our
simulation. This reactor will have a lifetime of 360 months (30 years),
cycle time of 15 months, assembly size of 30160 kg, and power capacity 1000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it would be better to make this some multiple of 6 months

source/user/tutorial/add_deploy.rst Outdated Show resolved Hide resolved
@abachma2
Copy link
Member

abachma2 commented Feb 23, 2021

Just pushed the final changes, specifically most of the comments from @gonuke, and adjustment of ZAID.

Add an accepted commodity for the repository to be Separated-Waste.

Activity: Save your Input File
--------------------
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this render right? Underline may not be long enough.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I build the pages locally, this section title renders the same as the others.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may give a warning and render fine...

@abachma2
Copy link
Member

I made Issue #307 to keep track of possible future changes to make, but this PR is complete and ready to merge pending approval from @gonuke.

Copy link
Contributor

@nuclearkatie nuclearkatie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @abachma2 and everyone else who contributed, this has been a long time coming! Very excited to see it live and be able to send new users to follow it

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sticking with this @abachma2 - Mars isn't the only one with perseverance!

@katyhuff katyhuff merged commit c33789c into cyclus:source Feb 25, 2021
@katyhuff
Copy link
Member

Merged! Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants