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.
There was a problem hiding this comment.
It actually writes the inputs and then runs mosden to generate results from those inputs. This is separate from the main mosden build, which is why there are results_generators in different example directories. The user is intended to run the results_generator to run many simulations one after another and have some nicely formatted figures and results once it finishes.
| '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.
There was a problem hiding this comment.
Yes! I could have renamed/restructured them, but I left them in the default structure as how they are when the download_data script collects them.
There was a problem hiding this comment.
Yes and no. For this example, this is an input. That is because it is four_dnps, so I have to provide the four DNPs it will model. For this case, it is Br87, I137, Rb94, and As86. The input file here is configured to not overwrite this file, so it will always be these four. If the user wanted to use other nuclides, they would simply need to alter this file to use the nuclides they want to see modeled.
| 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')?
There was a problem hiding this comment.
Yeah, I do have an issue (#21) that notes this. I think using a more consistent notation would be much better, but I think that may need to be its own PR.
| 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): |
There was a problem hiding this comment.
Oh so that checks if the concentration of the nuclide is close to 0.0 at all times, indicating that it has a negligible cumulative fission yield and does not needed to be tracked since it will emit approximately 0 delayed neutrons.
|
|
||
| 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
There was a problem hiding this comment.
Hm I see what you mean. Most of the preprocess functions are written this way though, since the purpose of the class is to preprocessing the files into the MoSDeN accepted formatting. Do you have suggestions for how I could make this a bit clearer?
|
|
||
| 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
There was a problem hiding this comment.
Oh darn you're right. Added Issue #62 to handle this at a later date.
| 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?
There was a problem hiding this comment.
Good question! It's a way of indicating privacy (see https://stackoverflow.com/questions/1547145/defining-private-module-functions-in-python). It doesn't change anything, but it signals that this function generally shouldn't be called elsewhere, but rather there is a different function that calls this function (jendl_preprocess in this case) that users should be using.
| 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.
There was a problem hiding this comment.
Good question! Generally no (see https://softwareengineering.stackexchange.com/questions/406570/pep8-d103-i-need-docstrings-for-my-unit-test-functions-too), but they should have sufficiently descriptive names.
| 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 |
There was a problem hiding this comment.
I had initially run an analysis that evaluated how varying the number of simulations affected this calculation, which showed that increasing the number of simulations improved this calculation. However, I think generally users are better off just utilizing the group values, so I have updated the documentation to indicate that.
|
@ElijahCapps Thanks for the review! Apologies for the length, I forgot how many new things I had included in this PR. Feel free to merge if it looks good, or let me know if there's anything you'd prefer I take another look at. |
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.