Add nuclear data for other datasets#53
Conversation
|
@ElijahCapps I've suggested you could be a good reviewer for this. Hopefully you agree. |
ElijahCapps
left a comment
ElijahCapps
left a comment
There was a problem hiding this comment.
Left a lot of comments in places that I had questions/suggestions on. This was a big PR, so I skipped over the .json inputs and .csv files, assuming they were formatted correctly. I glanced over the tests briefly, but did not look too deep into them. For additions to the core of Mosden (from what I could read), I mainly looked at new function descriptions/variable names to see if they made sense. Let me know if there is anything you would like me to look at closer. At some point, I think it might be a good idea if I tried to build this myself and see where it might hitch.
There was a problem hiding this comment.
I wonder if there is a better name for this than "results_generator" considering it generates inputs. Also, does this generate several inputs for several runs for one larger analysis? I am a little confused on what the input to this module actually is.
| 'overwrite': True, | ||
| }, | ||
| 'half_life': ['endfb71/decay/', 'endfb80/decay/', 'jeff311/decay/', 'jendl5/decay/', 'iaea/eval.csv'], | ||
| 'emission_probability': ['endfb71/decay/', 'endfb80/decay/', 'jeff311/decay/', 'jendl5/decay/', 'iaea/eval.csv'], |
There was a problem hiding this comment.
Are these the correct library directories? I am not familiar with how these libraries are structured.
| set(emission_nucs) & set(half_life_nucs) & set(conc_nucs)) | ||
| for nuc in net_similar_nucs: | ||
| data = conc_data[nuc] | ||
| Pn = pn_data[nuc]['emission probability'] |
There was a problem hiding this comment.
One convention uses spaces ('emission probability') and the other uses underscores ('half_life')?
| Pn = pn_data[nuc]['emission probability'] | ||
| hl = hl_data[nuc]['half_life'] | ||
| decay_const = np.log(2) / hl | ||
| if np.allclose(list(data.values()), 0.0): |
|
|
||
| def jendl_preprocess(self, data_val: str, unprocessed_path: str) -> None: | ||
| """ | ||
| Processes JENDL data |
There was a problem hiding this comment.
"Processes" is a bit of a vague descriptor
|
|
||
| def _jendl_nfy_preprocess(self, data_val: str, path: str) -> None: | ||
| """ | ||
| Processes JENDL fission yield data for the specified fissile target. |
There was a problem hiding this comment.
Minor nitpick but some descriptions have punctuation while others don't
| return None | ||
|
|
||
|
|
||
| def _jendl_nfy_preprocess(self, data_val: str, path: str) -> None: |
There was a problem hiding this comment.
Several functions begin with an underscore but many other do not. What does the format call for?
| index = base.get_irrad_index(False) | ||
| assert index == 3 | ||
|
|
||
| def test_get_irrad_index_with_min_time(): |
There was a problem hiding this comment.
Should tests have descriptions? I am not sure what the expectation here is.
| measures of the summed data than an in-core residence time of 30 seconds (30 | ||
| OpenMC simulations). | ||
| This can be resolved by using a smaller minimum OpenMC timestep. | ||
| The minimum timestep should be set such that approximately 100 simulations are |
3 participants
Summary of changes
Adds multiple new datasets and fixes some small bugs.
Types of changes
Associated Issues and PRs
Do not merge until PR #48 is merged.
Associated Developers
Checklist for Reviewers
Reviewers should use this link to get to the
Review Checklist before they begin their review.