Conversation
| cfy_in_yields = any(val in data['data_options']['fission_yield'] for val in self.cumulative_fission_yields) | ||
| if not cfy_in_yields: | ||
| raise ValueError(f"CFY requires cumulative fission yield data {self.cumulative_fission_yields} (not in {data["data_options"]["fission_yield"]})") | ||
| if data['modeling_options']['base_removal_scaling'] == 0.0: |
There was a problem hiding this comment.
Maybe this could be more general, for any value that is not between 0 and 1?
There was a problem hiding this comment.
Hmm, maybe? Again, this is handled much better in the next PR (once it is submitted) using the JSON schema. But specifically I want to point out to users that setting this value to 0 is not suggested (since it effectively implies the chemical removal rate occurs at a single point, which MoSDeN cannot currently model). I have added a line in Issue #40 to ensure that the JSON schema properly accounts for this.
| 'cross_section', | ||
| 'emission_probability'] | ||
|
|
||
| self.data_dir: str = file_options['unprocessed_data_dir'] |
There was a problem hiding this comment.
I wonder if there is a more concise way to initialize a class like this? Especially if many of the class variables are similar to the names used in the input file.
There was a problem hiding this comment.
Very good point! I have created Issue #42 pointing this out.
ElijahCapps
left a comment
ElijahCapps
left a comment
There was a problem hiding this comment.
Left a few comments over some minor issues, and a few questions in places I was confused. Otherwise, I think this does a great job cleaning up what was hard-coded before your prelim. Good job!
|
@ElijahCapps @bryanjp05 Thank you both for all your comments! Let me know if you still have comments, otherwise feel free to merge once you have both approved. |
ElijahCapps
left a comment
ElijahCapps
left a comment
There was a problem hiding this comment.
All new changes look good to me! I think this is ready to merge.
3 participants
Summary of changes
This PR cleans up the existing MoSDeN package based on previous feedback. This will enable a clean transition into the next stage of development focused on adding the new, higher fidelity model (see Issue #37).
This PR should not be merged until PR #36 is merged.
Types of changes
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.