-
Notifications
You must be signed in to change notification settings - Fork 66
Adding OpenMCCellTransform #1256
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
base: devel
Are you sure you want to change the base?
Conversation
|
Job Documentation, step Sync to remote on 7ef0664 wanted to post the following: View the site here This comment will be updated on new commits. |
|
Job Test nekRS on 5a186ff : invalidated by @meltawila |
|
Since @lewisgross1296 is planning to use cell rotations, how about we generalize this object so that it does both translations and rotations? I'm thinking in line of the |
| params.addParam<MooseEnum>("transform_type", | ||
| transform_type, | ||
| "Type of transform to apply: 'translation' (dx,dy,dz) or 'rotation'" | ||
| "(φ, θ, ψ) in degrees."); |
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.
do these symbols show up correctly in the documentation when compiled? also it may be clearer to say rotation angles about x, y, z because not everyone is consistent about these symbols
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 think this follows the notation listed in the openmc.Cell documentation. I agree that there's so many different versions of which angles are rotating about which axis over internet and textbooks, so it might be clearer to call them rot_x rot_y and rot_z? At the very least, we should reference the openmc.Cell documentation and make sure it's consistent with it
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.
The symbols show up correctly in the documentation, and yes I wanted to be consistent with OpenMC documentation but lmk if you prefer otherwise. The "the rotation angles in degrees about x, y, and z axes." description is already in docs. under example input syntax.
| params.addParam<MooseEnum>("transform_type", | ||
| transform_type, | ||
| "Type of transform to apply: 'translation' (dx,dy,dz) or 'rotation'" | ||
| "(φ, θ, ψ) in degrees."); |
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 think this follows the notation listed in the openmc.Cell documentation. I agree that there's so many different versions of which angles are rotating about which axis over internet and textbooks, so it might be clearer to call them rot_x rot_y and rot_z? At the very least, we should reference the openmc.Cell documentation and make sure it's consistent with it
|
The VTB test failure is unrelated |
|
Job Test VTB on f8908b8 : invalidated by @meltawila |
lewisgross1296
left a 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.
Nothing major, just some more thoughts I had
|
Job Precheck, step Clang format on a3af5f8 wanted to post the following: Your code requires style changes. A patch was auto generated and copied here
Alternatively, with your repository up to date and in the top level of your repository:
|
Co-authored-by: April Novak <novak@berkeley.edu>
Co-authored-by: April Novak <novak@berkeley.edu> Co-authored-by: Lewis Gross <43077972+lewisgross1296@users.noreply.github.com>
Co-authored-by: Lewis Gross <43077972+lewisgross1296@users.noreply.github.com>
5bd9b7b to
7ef0664
Compare
closes #1255