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.

Sign up for GitHub

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

Jump to bottom

Final Edits and Comparison with Cycamore #17

Merged
merged 63 commits into from
Sep 13, 2023
Merged

Final Edits and Comparison with Cycamore #17

merged 63 commits into from
Sep 13, 2023

Conversation

abachma2
Copy link
Collaborator

This PR got a bit bigger than I intended, so apologizes in advance.

This PR includes:

  • updates to the README on how to install dependencies and define a prototype using this archetype
  • remove files that aren't needed anymore from the examples directory
  • Update the archetype file (openmcyclus/DepleteReactor.py) include:
    • Updating datatypes, doc strings, and defaults for different state variables
    • Adding new state variables (or moving variables to be state variables) to match better with the Cycamore Reactor archetype
    • Adding capacities for the ResBufMaterialInvs
    • Request tracked materials
    • Defining requests to be exclusive (must be met in full, no partial fuel assemblies)
    • Remove recording information about the discharged assemblies to the database (no longer needed with the requested material being tracked)
    • Reflect changes to the Depletion class
    • Add function to record the position of the archetype to the database
  • Updates to the Depletion (openmcyclus/depletion.py) class include:
    • Reading in only materials instead of an entire model
    • Passing the updated materials from update_materials instead of writing to the materials.xml file
    • Update the run_depletion method to better match what is done in DepleteReactor
    • Add conversion of atom fraction to weight fraction to get_spent_comps()
  • Update the unit tests to reflect the changes in openmcyclus/depletion.py
  • Add a directory for the comparison with Cycamore:
    • comparison/Comparison-results.ipynb to hold the results of the comparison
    • comparison/OpenMC_model.ipynb to show how the cross sections were generated for OpenMCyclus, and the spent fuel compositions that were used in Cycamore
    • comparison/cycamore_recycle.xml: Cyclus input file using the Cycamore Reactor archetype
    • comparison/openmcyclus_recycle.xml: Cyclus input file using the OpenMCyclus DepleteReactor archetype

The CI is still broken, partly because of Issue #12. This will be investigated and updated in a future PR. The unit tests all pass locally. This PR should conclude the primary development of OpenMCyclus::DepleteReactor!

abachma2 added 30 commits June 26, 2023 16:23
@abachma2
create input for cycamore reactor
b8935a0
@abachma2
sdd file to create and run OpenMC model
7de6d5e
@abachma2
Merge branch 'main' into comparison
221b555
@abachma2
fix syntax errors, get sample spent fuel recipes in
84ce23a
@abachma2
update fresh recipes to match openmc model
719d693
@abachma2
update spent comps for 100 kg fuel mass
e4cf4b8
@abachma2
Merge branch 'depletion' into comparison
0cfc90c
@abachma2
update assembly volume to match 100 kg mass
280899f
@abachma2
checkout from depletion
21071be
@abachma2
checkout from depletion
e8ef58e
@abachma2
add input for openmcyclus use in comparison
66646c6
@abachma2
remove print statements
4781ea3
@abachma2
remove unused packages
0cce781
@abachma2
fix spelling error in directory and ntoebook file name
c200f4d
@abachma2
move lifetime to archetype, remove reciep for separations
0fc7e14
@abachma2
Merge branch 'main' into comparison
f25e660
@abachma2
remove decom_transmute_all statevar
bf8119c
@abachma2
add conditional to prevent early simulation termination when trading …
e40e28b 
…away spent fuel
@abachma2
create notebook for comparison analysis
ba4256b
@abachma2
Update spent_uox comp
6cc8beb 
Updated based on using a smaller power level to reach a burnup of 50 MWd/kgU and not fry the fuel trying to get 100 MW out of it.
@abachma2
remove conversion factor for power
ef94d8b
@abachma2
update spent compositions and power levels
3fcee91 
Previous power level of 100 MW and mass of 100 kg (arbitarily chosen)
were too high to correspond to a single pin cell model. Lower the power
level to match a burnup of 50 MWd/kgU, and the mass to
correspond to the volume and density of the fuel. Results still seem
a little funky, but it shoul dbe a more accurate/realstic simulation
@abachma2
debugging changes
e1e2156
@abachma2
change read_model to read_materials
c162e1e
@abachma2
remov unneeded files
1272613
@abachma2
update spent_uox comp
2bea502
@abachma2
pass a material object around instead of always reading from a file
d50a594
@abachma2
update openmc model
9d5e12f 
changed the power to a lienar power density (2-D geometry).
I also updated OpenMC, so the cross sections created by OpenMC
include many more nuclides. Updated the power for the prototypes
to match the needed power for the linear power density
@abachma2
remove print statements
0cb895c
@abachma2
revert transmutation to Cycamore method, broken
fb76e9e 
The compositions are correct, but the spent fuel isn't being
recorded as traded away.
@abachma2 abachma2 added the Type:Test Is related to software testing. Failing tests, necessary new tests, test frameworks, etc. label Jul 20, 2023
yardasol
yardasol requested changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@yardasol yardasol left a comment

Choose a reason for hiding this comment

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

Hi @abachma2, apologies for the late review on this.

in Comparison-results.ipynb, it is difficult to see the lines for the OpenMCylcus results in
your figures. I'd reccomend using a different line style (maybe try using dots: '.')

My other comments are below.

@abachma2 abachma2 requested a review from yardasol July 31, 2023 14:18
@abachma2
Copy link
Collaborator Author

To help speed up run times, I have added in some arrays to keep track of pre-depletion and post-depletion fuel compositions. If all of the pre-depletion compositions have been seen before (i.e., are in the pre-depletion composition array) then the corresponding post-depletion composition is applied to each of the assemblies instead of re-running the depletion solver.

smpark7
smpark7 requested changes Aug 23, 2023
View reviewed changes
Copy link

@smpark7 smpark7 left a comment

Choose a reason for hiding this comment

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

Good job with maintaining good documentation and tests.

Do you have any comments to add for differences observed in the comparison results plots? E.g. whether the differences are expected and why they appear.

My suggestions for other changes are provided below.

smpark7
smpark7 approved these changes Aug 23, 2023
View reviewed changes
Copy link

@smpark7 smpark7 left a comment

Choose a reason for hiding this comment

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

I approve but I assume we're waiting on @yardasol's comments. Remember to test locally again if you made new changes to your tests during the review period.

@abachma2
Copy link
Collaborator Author

abachma2 commented Sep 5, 2023

@yardasol Do you have any other comments or suggested changes for this PR?

yardasol
yardasol requested changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@yardasol yardasol left a comment

Choose a reason for hiding this comment

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

Hi @abachma2,

This is almost ready for merge. Looks like you resolved a couple of my comments without actually making any code changes. I've added additional in-line commments with some more context and unresolved those comments. Also, there are several code comments that should be deleted.

@abachma2
Copy link
Collaborator Author

Thanks for the feedback @yardasol. I think I have addressed your comments now (I must have missed a few from your last review, my apologies).

@abachma2 abachma2 requested a review from yardasol September 13, 2023 21:23
yardasol
yardasol approved these changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@yardasol yardasol 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. Just one more comment to remove then I can merge

@abachma2 @yardasol
remove last self.record, per @yardasol
948a723 
Co-authored-by: Olek <45364492+yardasol@users.noreply.github.com>
@abachma2 abachma2 requested a review from yardasol September 13, 2023 22:29
@yardasol yardasol merged commit 750d6e1 into arfc:main Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smpark7 smpark7 smpark7 approved these changes

@samgdotson samgdotson Awaiting requested review from samgdotson

@nsryan2 nsryan2 Awaiting requested review from nsryan2

@ZoeRichter ZoeRichter Awaiting requested review from ZoeRichter

@yardasol yardasol Awaiting requested review from yardasol

Assignees

@abachma2 abachma2

Labels
Comp:Analysis This issue has to do with the analysis component of the code or document. (plots, postprocessing) Comp:Core This issue has to do with the main bulk of the code or document. (methods, main content) Difficulty:2-Challenging This issue may be complex or require specialized skills. Priority:2-Normal This work is important and should be completed ASAP. Status:5-In Review This issue has been handled, and the solution is being reviewed. (probably as a PR) Type:Docs Is related to API documentation, website content, etc. Type:Feature New feature or feature request Type:Test Is related to software testing. Failing tests, necessary new tests, test frameworks, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abachma2 @smpark7 @yardasol