Skip to content
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

Merged
merged 17 commits into from
Sep 5, 2024
Merged

Cones #18

merged 17 commits into from
Sep 5, 2024

Conversation

valeriaRaffuzzi
Copy link
Member

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.

@Mikolaj-A-Kowalski Mikolaj-A-Kowalski added enhancement New feature or request Geometry labels Jan 19, 2023
@Mikolaj-A-Kowalski Mikolaj-A-Kowalski self-requested a review February 3, 2023 10:32
Copy link
Collaborator

@Mikolaj-A-Kowalski Mikolaj-A-Kowalski left a 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.

Geometry/Surfaces/Tests/cone_test.f90 Show resolved Hide resolved
Geometry/Surfaces/Tests/cone_test.f90 Show resolved Hide resolved
call self % setID(id)

! Set surface tolerance
call self % setTol(TWO * SURF_TOL)
Copy link
Collaborator

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.

Copy link
Member Author

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!

Copy link
Member Author

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.

Copy link
Collaborator

@Mikolaj-A-Kowalski Mikolaj-A-Kowalski left a 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.

Geometry/Surfaces/QuadSurfaces/cone_class.f90 Outdated Show resolved Hide resolved
Geometry/Cells/simpleCell_class.f90 Outdated Show resolved Hide resolved
Geometry/Surfaces/QuadSurfaces/cone_class.f90 Outdated Show resolved Hide resolved
Geometry/Surfaces/QuadSurfaces/cone_class.f90 Outdated Show resolved Hide resolved
Geometry/Surfaces/QuadSurfaces/cone_class.f90 Outdated Show resolved Hide resolved
Geometry/Surfaces/QuadSurfaces/cone_class.f90 Outdated Show resolved Hide resolved
Comment on lines 303 to 306
! 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
Copy link
Collaborator

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.

Copy link
Member Author

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!

Comment on lines 317 to 334
! 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
Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Member Author

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!

@valeriaRaffuzzi
Copy link
Member Author

valeriaRaffuzzi commented May 10, 2024

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.

Thanks a lot for reviewing this, I am a geometry beginner! I ran a couple of cases but not rayVol to be fair. I would say hold on with this, I will work on it a bit more during the weekend and fix everything you mentioned, and then do a bit more testing :)

Hopefully at least we're converging to something with these cones!

@valeriaRaffuzzi
Copy link
Member Author

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.

Thanks a lot for reviewing this, I am a geometry beginner! I ran a couple of cases but not rayVol to be fair. I would say hold on with this, I will work on it a bit more during the weekend and fix everything you mentioned, and then do a bit more testing :)

Hopefully at least we're converging to something with these cones!

I think rayVol might be broken. I tried to use it on regular JEZ and POPSY and I get an error: Coordinate list is not placed in the geometry, in move. We should open an issue about that. However, I tested the cones on a few configurations doing ST vs DT and they always agree perfectly! I also tried to make the unit test as varied as I could in terms of distance calculations. Let me know if there is anything else I could do to test this.

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 &
Copy link
Collaborator

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:

Suggested change
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.')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! On the plane of the basis
! On the plane of the base

!!
!! See surface_inter for details
!!
pure function evaluate(self, r) result(c)
Copy link
Collaborator

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! 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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cBasis = max(cMin, cMax)
cBase = max(cMin, cMax)

cBasis = max(cMin, cMax)

! Find the overall maximum
c = max(c, cBasis)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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
Copy link
Collaborator

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.

Suggested change
! 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
Copy link
Collaborator

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

Copy link
Member Author

@valeriaRaffuzzi valeriaRaffuzzi Sep 5, 2024

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

Copy link
Collaborator

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.

Copy link
Member Author

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?

Copy link
Collaborator

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! 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
Copy link
Collaborator

@ChasingNeutrons ChasingNeutrons Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! 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
Copy link
Collaborator

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?

Suggested change
! Inside the cone: basis intersection segment is -INF:+INF
! Inside the cone: base intersection distance is -INF:+INF

Copy link
Member Author

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.

Copy link
Collaborator

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! Well withn
! Well within

eps = -TWO * tolerance
@assertTrue(this % surf % halfspace(r + eps*u, u))

! Well Outside
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! Impacting the basis
! Impacting the bases

u = u/norm2(u)
@assertEqual(INF, this % surf % distance(r, u))

! Parallel to the basis
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! Parallel to the basis
! Parallel to the bases

ref = 3.0_defReal
@assertEqual(ref, this % surf % distance(r, u), TOL * ref)

! Parallel to basis
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
! Parallel to basis
! Parallel to bases

@@ -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
Copy link
Collaborator

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

Suggested change
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


Example: ::

billy { id 92; type xCone; vertex (1.1 4.0 2.98); angle 30; hMin 5.0; hMax 15.0; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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; }

Copy link
Member Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Copy link
Member Author

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

Copy link
Collaborator

@ChasingNeutrons ChasingNeutrons left a 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.

@valeriaRaffuzzi valeriaRaffuzzi merged commit d718945 into CambridgeNuclear:main Sep 5, 2024
5 checks passed
@valeriaRaffuzzi valeriaRaffuzzi deleted the Cones branch September 5, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Geometry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants