Refactoring moltres_xs to be able to use mgxs.Library() functionality. #340
Refactoring moltres_xs to be able to use mgxs.Library() functionality. #340Jeremyb8707bigbrain wants to merge 29 commits intoarfc:develfrom
Conversation
… tallies. Includes a built in .json dump file that can be removed/configured. Issue arfc#319
|
Hey everyone, I ran local tests myself for the godiva.json files, mostly everything lines up like it should. Another discrepancy that may be noted is the version, I am on OpenMC version 0.15.2 so the numbers are within std. dev, but for the smaller values it had a bit more range. 99% of everything works as it should, and I plan to add support for multiple statepoints and summaries soon. I added the openmc_mgxslib class so even if it is not 100% it can be added (maybe). Also I forgot to say, this one does not require an input file that specifies energy groups delayed etc, that all comes with a properly configured mgxs.Library(). Also I am working on ordering the files consistently in the.json. |
…ed sorting inside of the JSON.
…ned up spacing, extra lines, and more type checking and error catching.
…. Tweaked division by zero handling to be more readable and less complex.
… added back the consistent nu-scatter matrix requirement because I was testing stuff and forgot to add that back.
…sted the consistent nu-scatter matrix processing to allow all legendre_order types.
|
Hey @Jeremyb8707bigbrain thanks again for working on this PR. I have been very busy, but I have some general feedback:
|
|
Perfect I can do all of that! |
Changed main openmc (0.15.3) calls to run new function rather than the old one, added a way to manually build old case logic via inputs and inspecting python file for the mgxs.Library Added better logging for errors at mgxslib.load_from_statepoints.... Adapted it so that it will correctly use burn_idexes to comply with read_input() formatting. Set generate_openmc_talies_xml as a staticmethod explicitly
Local tests were ran and all files looked correct.
|
Hey @smpark7, everything for the most part is in order! I kept the openmc_xs class still available just in case you wanted to mark it for deprecation so users know that they will need to change their parameters slightly. I have a few questions, did you want me to include the SPN calculation? I can easily (I hope) include that I was just wondering if you would like me to. I also added TOTXS which is not present in the reference godiva.json file, but I see it in the code, would you like me to keep that or remove it? Last question, for your suggestion of having the mgxs.Library() be compatible with OpenMC's own multigroup mode, it would require an additional tally (Absorption), would you like me to add that and add the OpenMC Function check_library_for_openmc_mgxs() to the process_mgxslib() function to ensure advanced users must also meet that requirement? I did a few tests with multiple statepoints and summaries, here are the results, again I do not have access to all 8 delayed groups so some areas will be off very slightly. If you have any other concerns or questions please reach out! I also credit my research partner @rafainn for help with the input file work, very helpful! Example Python File: (I again dont have the cross sections at 900K or 1200K so I made them myself, which means sadly only 6 delayed groups) Input File: Output: I did another test with a different material, and same temperature and it worked as it should. |
importing where required rather than global to save compute for non-openmc based input functions Changed add_to_tallies_file to the newest non-deprecated version
|
Compatible until openMC 0.15.3 - since not mentioned explicitly elsewhere. |
|
Thanks @rafainn for also contributing to this PR. Yes SPN and TOTXS should be included. They're missing in the godiva files because the godiva test case was created before SPN and TOTXS were required for the new SN solver. (You can use this 1-D MSRE test case for an example that generates SPN and TOTXS) I see now that there are some incompatibilities between Moltres and multigroup OpenMC (OpenMC-MG) regarding required group constants:
One solution may be to stick with group constants that Moltres requires (as you have done) and let advanced users who want to use the same library for multigroup OpenMC add I think you need the JEFF cross section library to get 8 delayed groups, but that's not a big deal. |
…t becomes deprecated in a future update, because no version other than latest has that new name, and it still works with the latest version.
… for advanced users to match moltres input-file generated list.
|
@smpark7 I got the SPN Stuff down, adding new constants is quite easy with the library method. |
|
Thanks! I hope to have time next week to review this in detail. |
…houldnt differ but I believe this is safer and matches the older code better.
… from my other file.
smpark7
left a comment
There was a problem hiding this comment.
Overall this looks decent. Your library setup and calculations are consistent with the previous class, but there are some key issues to address:
- Multi-temperature group constant json file creation as a one-step process should be supported as a basic, not advanced, feature.
- The new class should either support the previous way of defining the json file structure for multiple temperatures, domains, and burnups (see how .inp files are read in
openmc_xs) or provide instructions for a new way. From my perspective, I don't know how exactly you used the append and dump functions to generate the json file for multi-temperature group constants. Imagine a new user tried to use this class. It should work exactly the same as the old class (and ideally replace it) or you have to provide a tutorial. - Check OpenMC version compatibilities; I doubt this will work for any version other than 0.15.3. I don't expect you to test it on every version (automated CI testing should be handling that). If you're not replacing
openmc_xs, you'll need to write tests (see $MOLTRES/python/test/)
General code comments:
- Eliminate empty lines as much as possible while following pep8 standards
- In
process_mgxslib, there are many intermediate variables that seem redundant e.g.nu_fission_sum,raw_nuscatter. Cut these down as much as possible.
I also added several other minor comments in my review.
If you're not planning to replace openmc_xs, you need to write tests and a tutorial to demonstrate how to use your new class.
| Energy groups to be used with OpenMC MGXS Tally creation, must include group edges. | ||
| delayed_groups: int | ||
| Delayed groups to be used with OpenMC MGXS Tally creation, must be an integer. | ||
| NOTE: May add support for lists for backwards compatibility at a later date. |
There was a problem hiding this comment.
Which versions does this break compatibility with? Is line 373-375 still true?
There was a problem hiding this comment.
Compatibility wise I saw that openmc_xs used a list instead of an integer for its delayed groups, which I wanted to cater to a little bit because I know the backwards compatibility aspect of this refactor is important.
There was a problem hiding this comment.
Would you like me to keep it as integer only? Or would you want me to accept ints and lists and just convert the lists to an integer. I only wanted to do this because I saw in the older code you used a list so users may not know to switch to an int perhaps? Just a thought
| def dump_json(self, json_path: str): | ||
| """ | ||
| This function is for advanced users who wish to bypass the input file. | ||
| Dumps the self.json_store dictionary into a JSON File. | ||
| Organization that matches reference JSON files is not 100% guaranteed. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| json_path: str | ||
| Output path for the JSON File. | ||
| Returns | ||
| ---------- | ||
| JSON File. | ||
| """ | ||
| if not self.json_store: | ||
| raise ValueError("JSON Store is empty") | ||
|
|
||
| with open(json_path, 'w') as f: | ||
| json.dump(self.json_store , f, indent=4) | ||
| print(f"Successfully Built and Dumped JSON at {json_path}") |
There was a problem hiding this comment.
What is this for? I don't think we need to cater to advanced users this way. They will know how to manipulate json files on their own.
| if version < 13.2: | ||
| raise Exception("moltres_xs.py is compatible with OpenMC " + | ||
| "version 0.13.2 or later only.") |
There was a problem hiding this comment.
See above comment on version compatibility
|
|
||
| # Sort temperatures to map them to branch indices deterministically | ||
| # Assuming temps are numbers, or convertible to float. None is treated as 294K conceptually or handled separately | ||
| temps_sorted = sorted(temps, key=lambda x: float(x) if x is not None else 294.0) |
There was a problem hiding this comment.
Is there any scenario where temps could be None?
There was a problem hiding this comment.
Yes I ran a few scenarios myself where the user does not explicity set a material or simulation temperature, and OpenMC defaults it to 294K. For some reason that also means in the library the temperature definition is left as None. May require further testing to make sure that wasnt just a me thing but it has happened which is why I put that check there
python/moltres_xs.py
Outdated
| if arr.ndim == 3: | ||
| arr = arr[:,:,0] | ||
| else: | ||
| print(f"Legendre Order of 0 Detected, Only P0 scattering is available for material: {mat_name} , results may differ from higher-order consistent scattering.") |
There was a problem hiding this comment.
ndim will always be 3 unless the user edits the python script
There was a problem hiding this comment.
I believe I put that check there for advanced users who may use their own library definition, would a P0 only Legendre still have an ndim of 3?
| if mgxs_type == "chi-delayed": | ||
| arr = arr.sum(axis = 0) | ||
| denom = arr.sum() | ||
|
|
||
| if denom!= 0: | ||
| arr = arr / denom | ||
| else: | ||
| arr = np.zeros_like(arr) |
There was a problem hiding this comment.
What is the normalization for?
There was a problem hiding this comment.
I noticed that when I did not normalize chi-delayed and only collapsed it, it never added up to 1 as it should and the numbers were off. After I did that normalization, it summed to 1 and matched the godiva reference data that I was working with. At the moment I cant tell you why that wasnt an issue for your code but I know that the normalization works here.
| self.json_store[mat_name][str(temp)][mgxs_key] = arr.tolist() | ||
| self.xs_lib[burn_idx][uni_idx][branch_idx][mgxs_key] = arr.tolist() | ||
|
|
||
| existing_temps = set(self.json_store[mat_name].get("temp", [])) # Added this so advanced users get the same temp structure if doing multi-file |
There was a problem hiding this comment.
Multi-temperature group constant generation should be a default feature.
There was a problem hiding this comment.
I believe I removed that line before then added it back because I was scared it would break things let me do some testing with that, and yes it is a default feature!
|
Thank you very much for this review, me and Raf will do our best to fix some things up. Multi-Temperature compatibility is a basic feature that everyone gets if they use the parser as done before, this is mostly handled by the parser and the reading of the input file, which automatically runs our process_mgxslib for each entry, so we snag the temperature that entry also. As a catering feature, I also added the functions dump_json and append_to_json, which is for more advanced users who do not wish to use the input parser. This may be a bit chunky and too much work just for a small group of users who don't want to use it, so removing those 2 functions would definitely save a lot of space, and just require everyone to use the parser. Example Beginner User Workflow: Example Advanced User Workflow: Writing that out it occurs to me that the advanced users workflow is a lot harder to do and catering more to beginners may work better with this function. If you would like for me to change/remove some of the advanced parameters/functions I would be happy to do so. Edit: |
|
Moose doesnt like my new file where I deleted openmc_xs, even though I have LF line endings and no whitespace. |
|
Thanks for continuing to work on this. I do see the value in your suggested "purely python input" approach as opposed to the current .inp input that was originally designed to cater to SCALE and Serpent workflows. When you're done with making changes to moltres_xs.py, please also provide an example workflow or tutorial that I may review as well. |
|
I would gladly do that, looking back on the file, the beginner workflow is much cleaner and uses preexisting infastructure so I will not be changing that. However the advanced workflow should be modified. If we are assuming advanced users have a good grasp on python and data structures in python, we could just have some sort of added input inside of the input file that would change the parameter There, instead of having to have those extra functions like So when I finish working and create a workflow and tutorial, expect dump_json and append_to_json to be gone and the workflow to be a lot simpler. Both methods will use the input file, and the advanced one will spit out the corresponding .h5 files that were used to create the json. That way advanced users have their proper json for Moltres, and if they want extra data in it or want to extract new data from their library, it would work. Would you prefer a more complicated, but all inside python workflow? Or still using the input file, but it is simpler? Edit: I have a working prototype of this without the extra dump and append functions, saved about 70 lines while also offering up each mgxs.h5 file. Example Input File for keeping the mgxs stores. [MAT] [BRANCH] [FILES] With the edition of the false parameter after each library file they would like to keep. This can be changed to whatever wording but false is what I had for testing |
Summary of changes
Types of changes
Required for Merging
Associated Issues and PRs
Associated Developers
Checklist for Reviewers
Reviewers should use this link to get to the
Review Checklist before they begin their review.