-
Notifications
You must be signed in to change notification settings - Fork 25
Wedge surface #176
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
Wedge surface #176
Conversation
ChasingNeutrons
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.
Looks good! Only a few queries or requests for a bit more clarity.
| @@ -0,0 +1,18 @@ | |||
| module compSurface_inter | |||
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 it necessary to make this? I think this just makes the code more complicated without adding functionality. It is appropriate to have composites in a folder, but the benefits of a subclass are unclear.
Maybe I should say the same thing about the quadSurface too...
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 guess it is useless for now, but it might add flexibility later. It might be good to reduce number of files for now so I am happy to remove both.
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 @Mikolaj-A-Kowalski remembers a reason for having quadSurface at least?
| @@ -1,10 +1,10 @@ | |||
| module cone_class | |||
| module truncCone_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.
Do we need the name change? I am a bit anti because it breaks csg2csg. Not major, but I get doing this more if we intend to make a regular cone afterwards.
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 like to keep the name consistent with the truncCylinder and self-explanatory. We can also consider making an infinite double-sided cone.
Hopefully this is just a search-replace in csg2csg!
Should be all done! |
| !! | ||
| subroutine kill_shortInt(self) | ||
| class(linkedIntList), intent(inout) :: self | ||
| integer(shortInt) :: i |
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.
Thank you for this!
This PR adds a wedge surface, with the properties as described in the docs. The wedge can be a bounding surface. In short:
All the unit tests pass; the validation cases I tried (mixing reflective and periodic BC, asymmetric cases, DT and ST), all using the wedge as a bounding surface, agree with a reference case made with a box surface.
I also collected the 'composite surfaces' (box, truncated cone and cylinder, wedge) in an appropriate folder.