-
Notifications
You must be signed in to change notification settings - Fork 25
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
Cones #18
Cones #18
Conversation
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.
Thanks for the contribution and sorry that surfaces are bit of the mess in SCONE.
The only major question I have is if wouldn't it be easier to just include both cones in the surface instead of trying to use only one? I guess it makes it more user friendly but it seems to require considerable complication when evaluating the surface expression or finding the distance.
call self % setID(id) | ||
|
||
! Set surface tolerance | ||
call self % setTol(TWO * SURF_TOL) |
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.
It probably should be made consistent with the value in the doc-comment.
Sadly there is a general problem with the surfaces and surface tolerance I missed when the current geometry was first made.
I wanted the surface tolerance to be well defined and be approximately a shell of constant 'fuzzy thickness' around the surface, but it sadly will not work with general quadratic surfaces where the 'radius' changes. Hence one cannot get away with the approximation of 2 * r * SURF_TOL
which worked well for cylinders and spheres...
I think it is fine to leave it as it is but perhaps there should be a comment that will say that 'the thickness of the fuzzy region around the surface will depend on the position on the surface and is not constant'.
We probably should open an issue about it as well. We need to rethink how the surface tolerance is done.
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 tests were a bit challenges because of this weird tolerance constant through a changing radius. Now everything should work but I agree that it's a bit annoying and suboptimal.
Hopefully this could be discussed in an issue and addressed in another PR!
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 changed the tolerance to be proportional to the 'mean radius' of the cone now. That is the radius of the cone at mid-height.
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 and thanks for the changes.
But because it is geometry it makes me super paranoid 😨 Did you run a lot of surface tracking calculations with the cones yet? We probably should construct geometries and torture them with the rayVol
physics package a bit before merging.
I will try to run some when I have some time.
! If one the distance found is smaller than the surface tolerance, then the particles | ||
! is on a surface: set that distance to infinity | ||
if (d1 < self % surfTol()) d1 = INF | ||
if (d2 < self % surfTol()) d2 = INF |
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 am not sure if this approach will work I'm afraid... :-(
The problem are particles hitting the surface at very shallow angle. They may be well within the tolerance but the distance to the surface may be still quite large. This is why in the e.g. 'cylinder' we need an extra case in the if-else for when a particle is within surface tolerance.
Sorry... geometry is super annoying.
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 see, I hadn't thought about that! I will reintroduce the extra if-else case when solving the quadratic, hopefully that will do it!
! Check that point found would be inside the cone | ||
testPoint1 = r(self % axis) + dist * u(self % axis) - self % vertex(self % axis) | ||
if (testPoint1 > self % hMax .or. testPoint1 < self % hMin) dist = INF | ||
|
||
! Calculate the distances from the basis of the cone | ||
d1 = (self % hMax - diff(self % axis)) / u(self % axis) | ||
d2 = (self % hMin - diff(self % axis)) / u(self % axis) | ||
|
||
! Verify if the points found would fall inside the cone | ||
testPoint2 = r(self % plane) + d1 * u(self % plane) - self % vertex(self % plane) | ||
testRadius = dot_product(testPoint2, testPoint2) | ||
maxRadius = self % hMax * self % hMax * self % tan2 | ||
if (testRadius > maxRadius) d1 = INF | ||
|
||
testPoint2 = r(self % plane) + d2 * u(self % plane) - self % vertex(self % plane) | ||
testRadius = dot_product(testPoint2, testPoint2) | ||
maxRadius = self % hMin * self % hMin * self % tan2 | ||
if (testRadius > maxRadius) d2 = INF |
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.
As an optimisation we can switch to the approach similar to the one used in the box
... basically it involves calculating the ranges on the line that fall within a particular body (e.g. inside of the cylinder or space between a pair of planes) and then performing the intersections on the ranges. But is is sadly a bit difficult to explain in the short comment ;-( We can talk about it in person perhaps.
That being said we can leave the explicit approach if it works well and include the alternative as a TODO in the 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.
No that's ok, over the weekend I will have a look at the box and fix this! But I would still need to calculate explicitly whether the particle would intersect the circles? Because in a box or cylinder the two faces are of the same size but this is not true 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.
Ok, the new distance calculation should be more like box and truncated cylinder now. The only thing I had to add is a check to make sure that the intersection found is on the right half-cone. Otherwise, the segment between two intersections could have been outside of the cone. If an intersection is on the wrong side of the cone, the distance is set to + or - infinity in a way that the intersection 'segment' (or ray) crosses the truncated cone (when here is an actual crossing). This might not be clear, in case let me know!
Thanks a lot for reviewing this, I am a geometry beginner! I ran a couple of cases but not Hopefully at least we're converging to something with these cones! |
I think |
end if | ||
|
||
if (angle <= ZERO .or. angle >= 90.0_defReal) then | ||
call fatalError(Here, 'Opening angle of cone must be in the range 0-90 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.
Maybe my Fortran is getting rusty, shouldn't this be:
call fatalError(Here, 'Opening angle of cone must be in the range 0-90 degrees & | |
call fatalError(Here, 'Opening angle of cone must be in the range 0-90 degrees '//& | |
'(extremes excluded). It is: '//numToChar(angle)) |
& (extremes excluded). It is: '//numToChar(angle)) | ||
end if | ||
|
||
if (hMin >= hMax) call fatalError(Here, 'hMin is greater or equal hMax.') |
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.
if (hMin >= hMax) call fatalError(Here, 'hMin is greater or equal hMax.') | |
if (hMin >= hMax) call fatalError(Here, 'hMin is greater than or equal to hMax.') |
aabb(self % axis) = self % vertex(self % axis) + self % hMin | ||
aabb(self % axis + 3) = self % vertex(self % axis) + self % hMax | ||
|
||
! On the plane of the basis |
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.
! On the plane of the basis | |
! On the plane of the base |
!! | ||
!! See surface_inter for details | ||
!! | ||
pure function evaluate(self, r) result(c) |
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.
Throughout this function I think 'basis' should be replaced with 'base' or 'bases'
pure function evaluate(self, r) result(c) | ||
class(cone), intent(in) :: self | ||
real(defReal), dimension(3), intent(in) :: r | ||
real(defReal) :: c, cMin, cMax, cBasis |
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.
real(defReal) :: c, cMin, cMax, cBasis | |
real(defReal) :: c, cMin, cMax, cBase |
c = dot_product(diff(self % plane), diff(self % plane)) - & | ||
self % tanSquare * diff(self % axis) ** 2 | ||
|
||
! Evaluate the expressions for the two basis of the cone and find the maximum |
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.
! Evaluate the expressions for the two basis of the cone and find the maximum | |
! Evaluate the expressions for the two bases of the cone and find the maximum |
! Evaluate the expressions for the two basis of the cone and find the maximum | ||
cMin = - diff(self % axis) + self % hMin | ||
cMax = diff(self % axis) - self % hMax | ||
cBasis = max(cMin, cMax) |
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.
cBasis = max(cMin, cMax) | |
cBase = max(cMin, cMax) |
cBasis = max(cMin, cMax) | ||
|
||
! Find the overall maximum | ||
c = max(c, cBasis) |
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.
c = max(c, cBasis) | |
c = max(c, cBase) |
! Vector for the difference between position provided and vertex | ||
diff = r - self % vertex | ||
|
||
! Evaluate the expression for the surface of the cone |
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.
All of the cone is arguably a surface.
! Evaluate the expression for the surface of the cone | |
! Evaluate the expression for the slanted surface of the cone |
c = dot_product(diff(self % plane), diff(self % plane)) - & | ||
self % tanSquare * diff(self % axis) ** 2 | ||
|
||
! Calculate delta/4 |
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.
What is delta? You mean the discriminant? That also does not appear to be what is calculated here - that would be b^2 / 4 - a*c
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 it's a bit confusion but the quadratic equation in this case is (comment in line 265): ad^2 + 2bd + a = 0
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.
Sure, but it isn't clear what delta is. If you say 'calculate delta/4' there is no way for someone reading afterwards to understand what you are referring to.
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.
Should I call it discriminant instead? Is this what you mean?
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.
Yes, that would be easier to follow.
! Calculate delta/4 | ||
delta = b * b - a * c | ||
|
||
! Calculate the distances from the cone surface |
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.
! Calculate the distances from the cone surface | |
! Calculate the distances from the slanted surface |
near = min(d1, d2) | ||
far = max(d1, d2) | ||
|
||
! Calculate the distances from the basis of the cone |
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.
! Calculate the distances from the basis of the cone | |
! Calculate the distances from the bases of the cone |
else ! Particle parallel to axis: check location | ||
|
||
if (diff(self % axis) > self % hMin .and. diff(self % axis) < self % hMax) then | ||
! Inside the cone: basis intersection segment is -INF:+INF |
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.
Should this be distance rather than segment?
! Inside the cone: basis intersection segment is -INF:+INF | |
! Inside the cone: base intersection distance is -INF:+INF |
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 segment is ok: each couple of distances (base1 and base2, or slanted1 and slanted2) identifies 2 points that create a segment. The intersection between the two segments is what is actually internal to the cone. This is what the code is doing, and I think the docs are consistent with the cylinder and box where this approach is used.
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 would then say the segment lies between -INF:INF. It is not clear what a segment being -INF:INF means.
d1 = -INF | ||
d2 = INF | ||
else | ||
! Outside the cone: basis intersection segment doesn't exist |
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.
! Outside the cone: basis intersection segment doesn't exist | |
! Outside the cone: base intersection distance doesn't exist |
|
||
end if | ||
|
||
! Save minimum and maximum distance from cone basis |
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.
! Save minimum and maximum distance from cone basis | |
! Save minimum and maximum distance from cone bases |
|
||
diff = r - self % vertex | ||
|
||
! Check the location of the particle, i.e., basis or cone surface, to calculate |
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.
! Check the location of the particle, i.e., basis or cone surface, to calculate | |
! Check the location of the particle, i.e., base or cone surface, to calculate |
@assertFalse(this % surf % halfspace(r, u)) | ||
|
||
! Out within tolerance | ||
! NOTE THAT the tolerance is very sensitive: if using eps = HALF * TOL, the |
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.
! NOTE THAT the tolerance is very sensitive: if using eps = HALF * TOL, the | |
! NOTE that the tolerance is very sensitive: if using eps = HALF * TOL, the |
eps = -HALF | ||
@assertTrue(this % surf % halfspace(r + eps*u, u)) | ||
|
||
! Well withn |
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.
! Well withn | |
! Well within |
eps = -TWO * tolerance | ||
@assertTrue(this % surf % halfspace(r + eps*u, u)) | ||
|
||
! Well Outside |
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.
! Well Outside | |
! Well outside |
u = u /norm2(u) | ||
@assertTrue(this % surf % halfspace(r, u)) | ||
|
||
! Choose point on one of the basis of the cone, moving out |
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.
! Choose point on one of the basis of the cone, moving out | |
! Choose point on one of the bases of the cone, moving out |
ref = 4.0_defReal | ||
@assertEqual(ref, this % surf % distance(r, u), TOL * ref) | ||
|
||
! Impacting the basis |
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.
! Impacting the basis | |
! Impacting the bases |
u = u/norm2(u) | ||
@assertEqual(INF, this % surf % distance(r, u)) | ||
|
||
! Parallel to the basis |
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.
! Parallel to the basis | |
! Parallel to the bases |
ref = 3.0_defReal | ||
@assertEqual(ref, this % surf % distance(r, u), TOL * ref) | ||
|
||
! Parallel to basis |
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.
! Parallel to basis | |
! Parallel to bases |
docs/User Manual.rst
Outdated
@@ -517,6 +517,23 @@ Example: :: | |||
|
|||
billy { id 92; type xCylinder; origin (0.0 0.0 9.0); radius 4.8; } | |||
|
|||
* cone: cone aligned with x, y or z axis, and truncated arbitrarily on both sides. | |||
The input type has to be ``xCone``, ``yCone`` or ``zCone``. The orientation of the |
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.
Orientation is ambiguous with x, y, and z
The input type has to be ``xCone``, ``yCone`` or ``zCone``. The orientation of the | |
The input type has to be ``xCone``, ``yCone`` or ``zCone``. The gradient of the |
docs/User Manual.rst
Outdated
|
||
Example: :: | ||
|
||
billy { id 92; type xCone; vertex (1.1 4.0 2.98); angle 30; hMin 5.0; hMax 15.0; } |
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.
billy { id 92; type xCone; vertex (1.1 4.0 2.98); angle 30; hMin 5.0; hMax 15.0; } | |
connor { id 92; type xCone; vertex (1.1 4.0 2.98); angle 30; hMin 5.0; hMax 15.0; } |
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.
Did you change name to Connor because it sounds like cone?
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.
Yes
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.
Fair enough. I love bad humor
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.
All small things I think. Should be straightforward.
Co-authored-by: Paul Cosgrove <pmc55@cam.ac.uk>
Here's a cone quadratic surface. It's an infinite cone, aligned with the x, y or z axis.
The input definition is the same as MCNP (need to specify the tangent of the opening angle and the orientation of the cone), compatible with Serpent as well.
I wrote a unit test and compared my results with Serpent, which agree.