CylinderSector composite surface
#2027
Conversation
|
Bump since this has been sitting for a while @paulromano @pshriwise |
|
@yardasol Apologies! I was on vacation last week and just now catching up. I'll try to take a look this week |
There was a problem hiding this comment.
Thanks for another contribution @yardasol! A few simple comments and I think this is good to go. Sorry it took so long to get you a review on this!
openmc/model/surface_composite.py
Outdated
| theta0 : float | ||
| Angular offset of the sector in degrees relative to the primary basis | ||
| axis (+y, +z, or +x). | ||
| theta : float | ||
| Angular width of the sector in degrees. |
There was a problem hiding this comment.
To prevent confusion, I think we should have the user specify two angles theta1 and theta2 rather than an angle and an angular width.
There was a problem hiding this comment.
I think having an offset and a width is a bit more robust of an approach than two explicit angles, but I agree that having two theta variables is confusing... maybe delta would be a good variable for the angular width?
I actually initially thought about using a two-angle approach like you suggested, but one of the issues I ran into was handling cases where the two angles are inverted, i.e. theta1 = 30 and theta2 = 40 would be a 10 degree sector, whereas theta1 = 40 and theta2 = 30, would be a 350 degree sector, but this isn't entirely clear from just reading off the values of theta1 and theta2. The offset and sector width formulation is clearer about this difference.
One snag I can see right now though is it's not entirely clear where on the sector the offset is measured to. In my head it's the clockwise-most edge of the sector but this isn't specified in the docstring.
There was a problem hiding this comment.
One way to get rid of any ambiguities with two angles is to require that theta2 > theta1
There was a problem hiding this comment.
I've changed the variable names to be more specific. Instead of theta we have central_angle, and instead of theta0 we have ccw_offset. I've also changes the internal variables theta0 and theta1 to phi1 and phi2, respectively.
…ariables less ambiguous
… for theta,alpha formulation
There was a problem hiding this comment.
A few more minor things and then this should be good to go. Thanks @yardasol!
…different constructors
|
Thanks for another contribution @yardasol! |
This PR adds a new
CompositeSurfaceclass,CylinderSector, and associated unit tests. This PR also adds theCylinderSectorclass to the SphinxDocsThis PR also contains some pep8 fixes generated using
autopep8