Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding OpenmcReader and Pebble Classes #1
Adding OpenmcReader and Pebble Classes #1
Changes from 4 commits
25fcd34
970356a
16e3187
6230449
3e4f1e3
fd4910b
e3da9d2
15a63ef
c940e20
e1823b2
b48cc8c
0cbc31e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a more descriptive name for this file that would work better? reader_openmc? or just readers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @nsryan2's comment. I might offer
mc_reader.py
standing formonte carlo reader
which could be OpenMC or Serpent.@ZoeRichter if you plan to have more than one reader, you could consider borrowing some of @yardasol's code in Saltproc for reading data from Serpent/OpenMC (and others?) Maybe I'm misremembering the purpose of
depcode
@yardasol.Regardless, the pattern I'm suggesting is to have a generic reader object and have the openmc or serpent specific readers inherit from it and use the
super().__init__()
function (seeosier.Technology
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was on the fence about this specific implementation, actually. I initially started by creating it the way you suggested - by having a parent reader that's generic, and child classes for code-specific readers. But as I was making it, I realized the ones I knew enough about to be able to implement right now didn't actually have args in common, so I made the reader(s) distinct. I think going the other way (converting didymus to a given code format) will work best in that parent-child set up, however. Do you think it would still be best to organize them under an empty parent class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a little unclear here, is it an object for each x, y, z set or each x coordinate, each y coordinate, and each z coordinate? The former, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, it's a pebble object for each set of (x,y,z) coordinates in coord_array (each set corresponds to a specific pebble's center). Do you think "...for each centroid coordinate provided..." would be a better descriptor (bouncing off of Sam's suggested change in a later comment)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List comprehensions are faster. Is every pebble going to be a unique material? Should mat_ids be part of the initialization of the
OpenMCReader
? It seems cumbersome (and error prone) to have users generate id's for each pebble themselves.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each pebble will not necessarily be a unique material, I think it's more likely that you will have large numbers of the same pebble material (such as having a unique material description for each pass the pebbles have made through the core). I don't think having a default for the uniq_ids/pebble_ids is a bad thing, though.