-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add method to get toroidal models, add parastell tool, overhaul text wrapping, allow more generalized text in plot #13
Conversation
…d, with parastell method
Let's remember to keep individual PRs small and focused :) |
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.
A few suggestions here, but this looks like a great topic for a software meeting
radial_build.py
Outdated
f'{layer["description"]}', | ||
self.max_characters, | ||
drop_whitespace=False | ||
) |
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.
move rstrip()
here
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.
since either, both, or neither of the "composition"
and "description"
keys may be present the rstrip()
works best in its current location, I think its the simplest, otherwise logic needs to be in place to check what combination of keys were specified.
radial_build.py
Outdated
name=layer, | ||
fill=layer_def['material']) | ||
materials.append(layer_def['material']) | ||
except KeyError as e: |
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.
test input validity at beginning of function and then cleaner code with guaranteed valid input
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.
splitting these into separate classes makes me happy to just not check input for now, I will defer to your judgement however
I made updates incorporating suggestions from the last round of reviews, adding the ability to specify a list of materials for each layer along with volume fractions and have them be mixed, into a new material, along with tweaking the command line interface so that yml files can be used for both toroidal models and radial build plots. These changes are here, Edgar-21#1, I can just merge them to this branch if that is more convenient. I split it out for now so it is more obvious what changes have been made since the last review. |
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.
A few more changes in a final(?) cleanup
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.
One question in the example....
This is branched off the branch in #11 (superseding it), and includes the
from_parastell_build()
method. I was doing some things with toroidal geometries today and felt it would be a good time to wrap that tool into this.Plotting
I renamed
plot_radial_build.py
toradial_build.py
to reflect that the script now does more than just make the plots. The main class,radial_build()
now only requires the 'build' dict to initialize. Relevant plotting parameters can be passed to theplot_radial_build()
method, and are available throughout the class. These have a default value ofNone
until the plotting function is used but I think that will be ok since I don't imagine these parameters getting used for anything aside from plotting.Model Building
A method that returns a model with toroidal geometry that reflects the
build
dict was added. If it is desired to use this method the build dict must have a"material"
corresponding to an openmc material. This is indicated in the docstring, and if it is not present the method should throw a helpful error. Also returned is a dictionary mapping thebuild
keys (layer names) to corresponding cell instances, which is useful for defining filters later.Examples
Updated the examples to use the new function calls, and added an example for using the toroidal model builder.
closes #10
closes #5