Conversation
|
thanks for the catchup just now @gschnabel I made the changes to the output folder and this PR is now ready for review |
gschnabel
left a comment
gschnabel
left a comment
There was a problem hiding this comment.
See the comments. Let me know if you want to implement the changes or prefer me doing it. In the latter case, I may need help with the openmc.data interface.
| ln -s /opt/NJOY2016/bin | ||
| ln -s /opt/NJOY2016/bin && | ||
| cd /opt && | ||
| git clone --branch master https://github.com/openmc-dev/openmc.git && |
There was a problem hiding this comment.
Please pin a specific commit of OpenMC (as done for NJOY2016) for traceability and reproducibility.
| '.', njoyexe, njoylib, njoyvers, fendlvers, cdate, endf_file=endf_file | ||
| ) | ||
|
|
||
| if 'hdf5' in formats: |
There was a problem hiding this comment.
My understanding is that openmc.data produces ACE files on-the-fly with its own defaults for NJOY as a prerequisite for the conversion to hdf5. This is one reason why the split here is problematic because the ACE file produced in the branch above would be inconsistent with the ACE file used for the hdf5 creation (e.g. different temperatures).
Therefore, I suggest giving up on format selection at this level and instead do the following:
Add the hdf5 output path to the
fendl_paths['output'] dict in determine_fendl_paths and extend the code in run_fendl_njoy to produce an hdf5 file, ideally using the ACE file generated by our custom FENDL NJOY in the same function. This also means that the additional module process_fendl_neutron_hdf5.py script should be removed.
| process_fendl_proton_lib( | ||
| '.', njoyexe, njoylib, njoyvers, fendlvers, cdate, endf_file=endf_file | ||
| ) | ||
| if library_type in ('photon', 'all'): |
There was a problem hiding this comment.
This conditional branch is okay. For streamlining, I suggest keeping exactly the same function signature as for the other fendl_process_*_lib functions used above. Then, it will also be nice to use the same internal logic within process_fendl_photon_hdf5.py as used in the other process_fendl_*.py files. Regarding this, see my comments added to process_fendl_photon_hdf5.py.
There was a problem hiding this comment.
When I look at the PR locally, I see that this is a regular file but should be a symbolic link. I guess there was some issue with git-annex. I've usually encountered these issues on Windows. Anyway, this is a minor thing that we can fix at the end.
There was a problem hiding this comment.
I suggest removing this file and implement its logic in process_fendl_neutron.py as described in this comment
There was a problem hiding this comment.
All the other process_fendl_*.py modules follow a generic structure. They contain the functions run_fendl_njoy and determine_fendl_paths and then a main function process_fendl_neutron_lib (which will be named process_fendl_photoatomic.py or something like this) which calls the generic function process_fendl_sublib with arguments including the run_fendl_njoy and the determine_fendl_path functions. It would be nice to maintain the same kind of logic in the process_fendl_photoatomic.py (or similarly named file).
@shimwell Sure, some basic verification with a transport code would be a good idea to catch issues early on. |
- add process_fendl_photoatomic.py matching neutron/proton/deuteron layout (run_fendl_njoy, determine_fendl_paths, process_fendl_photoatomic_lib) - remove process_fendl_photon_hdf5.py - allow process_fendl_endf to tolerate modules without NJOY inputs/outputs - use uniform *_lib signature for photo-atomic in process_fendl.py
|
Hi @gschnabel — sorry it's taken so long to get back to this. I've pushed commits that try to address the review comments:
Happy to make any further adjustments needed to get this merged. |
2 participants
This PR proposes to add processing of fenld endf files into hdf5 openmc files
I am adding as a draft as I would like to test the hdf5 files a bit more to be confident. On that topic can i ask, is testing the hdf5 files in particle transport something that this repo would be a good home for (I can add some CI to do that)?