Repository navigation
Conversation
This reverts commit cba8004.
…sing info to output
2025 sai emanes5 See merge request system-analysis-and-integration/watts!3
…tegration/watts into cyclus_plugins
Cyclus plugins See merge request system-analysis-and-integration/watts!4
Add Cyclus plugin demo notebooks and setup script; update plugin_cyclus See merge request system-analysis-and-integration/watts!5
Dymond edits See merge request system-analysis-and-integration/watts!1
Cyclus plugin streamlining core See merge request system-analysis-and-integration/watts!22
Add documented Cyclus and DYMOND plugin tests See merge request system-analysis-and-integration/watts!23
…fuel cycle utilities as well
Co-authored-by: Ryan, Nathan <ryanns@ornl.gov>
Plugins overhaul See merge request system-analysis-and-integration/watts!24
Changed plugin See merge request system-analysis-and-integration/watts!25
|
@nstauff, here are the additions to Watts we developed for the SA&I campaign, to be used with WISE. I would appreciate if we could work together to make this mergeable if any issue comes up. Thank you! |
|
@abachma2 and @JiaZhou-PU - Could you add yourself as reviewer and confirm you are OK with this PR? |
|
I don't see a way to add myself as a reviewer -- I don't see that option. I will review this later this week, likely on Friday. My review will focus on the Cyclus and DYMOND additions/changes, Jia can speak more to the ACCERT changes. Feel free to ping me if you don't see anything by next week. |
|
@nstauff I am good with those changes in ACCERT, although I will submit another PR based on the recent ACCERT version. |
| """Total OCC = Total cost without owner + Owner cost""" | ||
| return (self.account_table['total_cost'].values[0] * 1.609 * 1.2 / | ||
| 0.834) | ||
|
|
There was a problem hiding this comment.
This is the default PWR algorithm for the OCC in ACCERT. I will expand on those later
nstauff
left a comment
There was a problem hiding this comment.
Thanks - see my submitted comments. Flagging for @paulromano to complete a review and discuss how best organize the tests and utilities within WATTS.
| # SPDX-License-Identifier: MIT | ||
|
|
||
| """ | ||
| This example demonstrates how to use WATTS to perform a criticality search on an |
There was a problem hiding this comment.
please update the description here - this appears to be a bad copy/paste
| params = watts.Parameters() | ||
|
|
||
|
|
||
| # Create MCNP plugin |
There was a problem hiding this comment.
please update comment. - this is cyclus plugin...
| # Check for environment variable | ||
| base_dir = os.environ.get(environment_variable) | ||
| return base_dir / exe if base_dir is not None else exe | ||
| if base_dir is None: |
There was a problem hiding this comment.
I am not sure to understand what those changes to plugin do, and what motivates those. Flagging for @paulromano review.
| @@ -0,0 +1,427 @@ | |||
| import numpy as np | |||
There was a problem hiding this comment.
@paulromano - please let me know how you want this watts source code to include additional utility files.
| @@ -0,0 +1,211 @@ | |||
| import pytest | |||
| import numpy as np | |||
There was a problem hiding this comment.
I recommend renaming the test to make it more explicit - this is a test on utility "enrichment" features. Some description/comments would be useful as well.
There was a problem hiding this comment.
I recommend changing the file name to test_enrichment_utils.py to make it more clear that you are testing the functions in the enrichment_utils.py file.
abachma2
left a comment
There was a problem hiding this comment.
I'm still working through the files, it's taking me longer than expected, but I wanted to get you some feedback in the mean time. A few major items:
- Two tests fail for me locally:
tests/test_fileutils.py, test_run(only on my Windows machine) andtests/test_parameters.py, test_parameters_show_summary(on my Windows and Linux machines). - I recommend going through the comments you have and seeing what is necessary. The code I have seen is quite cleanly written, so comments should be more about why you are doing something and less on what the code is actually doing. I flagged a few cases of this.
- It seems like the cyclus and dymond examples are quite different -- they even have a different number of variable parameters in the template. These codes (for the purposes of a versatile tool like WATTS) do the same things and it might make sense for them to have similar examples.
I'll keep working on the rest of my review next week.
| <fuel_outrecipes> <val>lwr_used_fuel_recipe</val> </fuel_outrecipes> | ||
| <fuel_incommods> <val>enriched_u</val> </fuel_incommods> | ||
| <fuel_outcommods> <val>waste</val> </fuel_outcommods> | ||
| <fuel_prefs> <val>0.5</val> </fuel_prefs> |
There was a problem hiding this comment.
What is the purpose of having this fuel pref?
| <nuclide> <id>942410000</id> <comp>36.477</comp> </nuclide> | ||
| <nuclide> <id>942420000</id> <comp>15.387</comp> </nuclide> | ||
| <nuclide> <id>952410000</id> <comp>1.234</comp> </nuclide> | ||
| <!-- <nuclide> --> <!-- <id>95242m</id> --> <!-- <comp>0.03</comp> --> <!-- </nuclide> --> |
There was a problem hiding this comment.
Any reason this nuclide is commented out?
| @@ -0,0 +1,18 @@ | |||
| # 1App_MCNP_Jezebel | |||
There was a problem hiding this comment.
| # 1App_MCNP_Jezebel | |
| # 1App_Cyclus |
|
|
||
| ## Purpose | ||
|
|
||
| This example provides a demonstration on how to use WATTS to run Cyclus. |
There was a problem hiding this comment.
Should you add a description of what the example problem is?
| ## File descriptions | ||
|
|
||
| - [__watts_exec.py__](watts_exec.py): WATTS workflow for this example. This fills the duration of the Cyclus run. | ||
| - [__cyclus_template.xml__](cyclus_template.xml): Cyclus input file with duration template. |
There was a problem hiding this comment.
| - [__cyclus_template.xml__](cyclus_template.xml): Cyclus input file with duration template. | |
| - [__cyclus_template.xml__](cyclus_template.xml): Cyclus input file template with simulation duration as a variable. |
This wording feels more clear to me.
| for n in range(n_stages - 1, -1, -1): | ||
| swu = qty_prod[n] * swu_to_product_ratio( | ||
| feed_enrs[n], prod_enrs[n], tails_enrs[n] | ||
| ) |
There was a problem hiding this comment.
In this loop it feels like you have some variable assignments that maybe aren't necessary. LIke maybe move the swu calculation to the end and assign it directly into the swu_list variable, and then have the printing at the very end.
| ) | ||
| cascade_data = {} | ||
| for key in multistage_dict: | ||
| cascade_data[key] = list(multistage_dict[key]) |
There was a problem hiding this comment.
Isn't this already a list?
|
|
||
| # DYMOND process times use a Julian year. Defining it here keeps both plugins | ||
| # on the same time conversion when they report common WATTS results. | ||
| SECONDS_PER_YEAR = 365.25 * 24 * 60 * 60 |
There was a problem hiding this comment.
Maybe something later on will help elucidate how time is handled in all of this, but:
- This value doesn't match the number of seconds per year in Cyclus when using the default time step. The default time step in Cyclus is 2,629,46 seconds, or 1/12th of a year and that times 12 does not equal this variable.
- Cyclus allows for a variable time step size -- do you have a way to account for that in the processing?
| Attributes | ||
| ---------- | ||
| obj : CyclusOutput | ||
| Reader for the Cyclus SQLite file. The file is opened when this |
There was a problem hiding this comment.
Cyclus can (in theory) yield results databases in other formats, including HDF5. Does this class only work with SQLite databases? If so, that should be documented.
| # WATTS moves the SQLite file after this object is created. Open the | ||
| # file only when a caller first requests ``obj`` so the reader uses the | ||
| # final path and WATTS never tries to save an open SQLite connection. |
There was a problem hiding this comment.
Does this need to be here, or should it be in the docstrings?
abachma2
left a comment
There was a problem hiding this comment.
I finally got a chance to look through everything. Thanks for your patience. A few big items/themes:
- I would check on the vocabulary used in each plugin and make sure it matches the correct vocabulary for each code. It might confuse a user if vocabulary isn't consistent with what each code uses.
- There are multiple places where the first part of a function looks to be changing the datatype of one of the inputs, and it isn't clear to me why you would do that instead of just requiring the parameter to be of that datatype.
- It looks like there are places in which you want the Cyclus and DYMOND plugin to match perfectly, which sometimes leads to mixing of vocabulary. Do the plugins need to match perfectly? Can they have functions that do the same thing, but are called different things because of different vocabularies?
- I can run the Cyclus example locally, but I can't get the DYMOND example to run. It looks like it creates the input file correctly, but them dymond can't seem to run the file and I can't know why. I'll keep looking into this one to see if I can figure anything else out.
- Is there any reason you didn't update the CHANGELOG file for this PR?
| def find_single_output( | ||
| outputs: Iterable[Path], | ||
| suffixes: Sequence[str], | ||
| description: str, | ||
| ) -> Path: |
There was a problem hiding this comment.
This function is meant to find if an output file exists, and get that file path, correct? As written, it feels too complicated for this task. Can you provide some context/information on why it's written this way?
| return str(unit).strip().upper().replace(" / ", "/") | ||
|
|
||
|
|
||
| def power_to_gw(values, unit: str) -> np.ndarray: |
There was a problem hiding this comment.
Any reason you selected GW as the unit to convert things to? As opposed to MW?
| If a requested reactor is not present in ``available``. | ||
| """ | ||
|
|
||
| available_names = tuple(str(name) for name in available) |
There was a problem hiding this comment.
Why not just require that available be defined as a tuple? That would remove the need to define another variable and to assign a new datatype to it.
| # A dictionary preserves insertion order, so it removes repeated names | ||
| # without changing the order supplied by the caller. | ||
| requested = tuple(dict.fromkeys(requested)) |
There was a problem hiding this comment.
I thought dictionaries were unordered, which is why you can't get a value based on index. So I'm a little confused on the purpose of this part.
| def aggregate_calendar_years( | ||
| values, | ||
| init_year: int, | ||
| init_month: int, | ||
| how: str, | ||
| ) -> tuple[np.ndarray, np.ndarray]: | ||
| """Combine monthly values by calendar year. |
There was a problem hiding this comment.
Does this function have baked into it the idea that each timestep is 1 month? Does it allow any flexibility for if a timestep isn't 1 month?
| Parameters | ||
| ---------- | ||
| reactors : str or list[str] | ||
| Prototype name(s). |
There was a problem hiding this comment.
I don't think dymond uses "prototype" in it's vocabulary. Is there another term that can be used here?
|
|
||
| return collected | ||
|
|
||
| # =================================DEPRECATED================================== |
There was a problem hiding this comment.
Do we need to keep the deprecated functions?
| def _normalize_facilities( | ||
| self, | ||
| facilities, | ||
| facility_class="reactor", |
There was a problem hiding this comment.
Does it make sense to have a default value for this parameter?
| @pytest.fixture | ||
| def dymond_output(tmp_path): | ||
| """Create a compact but representative DYMOND HDF5 output. |
There was a problem hiding this comment.
Would it be easier to just store an output database for testing?
| cascade_splits=[], | ||
| ) | ||
|
|
||
| cascade_5 = enrichment_cascade( |
There was a problem hiding this comment.
Are you using another function to test the output of this one? Does the one function use the enrichment_cascade function?
abachma2
left a comment
There was a problem hiding this comment.
I finally got a chance to look through everything. Thanks for your patience. A few big items/themes:
- I would check on the vocabulary used in each plugin and make sure it matches the correct vocabulary for each code. It might confuse a user if vocabulary isn't consistent with what each code uses.
- There are multiple places where the first part of a function looks to be changing the datatype of one of the inputs, and it isn't clear to me why you would do that instead of just requiring the parameter to be of that datatype.
- It looks like there are places in which you want the Cyclus and DYMOND plugin to match perfectly, which sometimes leads to mixing of vocabulary. Do the plugins need to match perfectly? Can they have functions that do the same thing, but are called different things because of different vocabularies?
- I can run the Cyclus example locally, but I can't get the DYMOND example to run. It looks like it creates the input file correctly, but them dymond can't seem to run the file and I can't know why. I'll keep looking into this one to see if I can figure anything else out.
- Is there any reason you didn't update the CHANGELOG file for this PR?
- It looks like the license text on some of the files says 2024 and others say 2026. Any reason for the difference?
Description
This pull requests supports the use of Watts as part of WISE and the Systems Analysis and Integration Campaign.
This pull request adds support for Cyclus and DYMOND within WATTS and introduces a shared infrastructure for common nuclear fuel cycle analyses.
The primary additions include:
A new Cyclus plugin for executing Cyclus simulations and reading SQLite output.
A new DYMOND plugin for executing DYMOND simulations and reading HDF5 output.
Shared fuel-cycle utility modules that provide a common implementation between the two plugins for:
Additional ACCERT result properties exposing direct, indirect, owner, and total overnight capital cost (OCC) metrics.
Registration of the new plugins within the WATTS plugin framework.
Comprehensive unit and integration tests covering the new functionality.
The shared fuel-cycle infrastructure was created to reduce duplicated logic between the Cyclus and DYMOND plugins while providing a consistent public interface for common nuclear fuel cycle analyses.
Testing
tests/test_fileutils.py::test_run) fails because it invokes the Unixenvexecutable; this failure is unrelated to the changes introduced in this pull request.Checklist