Initial portable implementation of ORANGE: oak ridge geometry engine #291

sethrj · 2021-10-20T11:55:51Z

This will be the first set of functionality for an in-house geometry (see #267 ). It will include:

On-device intersection, sense evaluation, and surface normals for implemented surfaces
Axis-aligned plane, axis-aligned centered cylinder, and general quadric shape
Type-deleted surface storage and templated generic actions

paulromano · 2021-10-20T12:00:14Z

The most important question — what is the "AN" in "ORANGE"?!

sethrj · 2021-10-20T12:06:14Z

Bwaha this was a discussion topic for a while! I want it to be "Arbitrarily Named" or "Acronym-Neutral", but the vote was to call it "Adaptable Nested"...

sethrj · 2021-10-21T01:14:19Z

Type-deleted collections-based surfaces are working! Next is to add a GPU test :) and then I think this phase will be ready to merge (unless I should keep going on this branch...)

sethrj · 2021-10-21T11:46:05Z

Pre-merge question probably for @tmdelellis : how should we namespace this package? SCALE's version uses an orange namespace and pulls stuff from geometria; currently we're using celeritas here for simplicity. Since this refactor of ORANGE will require Celeritas core components (Collections, macros, Span, Array) should we assume for the moment that the same namespace is ok? Long term do we want to move ORANGE and the Celeritas core components to a separate package that allows downstream codes to install only the geometry/core features?

src/orange/surfaces/GeneralQuadric.hh

src/orange/Types.hh

sethrj · 2021-10-25T20:13:19Z

@tmdelellis et al: I think this is ready for review as the first phase of geometry implementation. I'll have another PR for transporting among volumes, then another one for Celeritas integration, then another for multiple universes etc.

Before: ``` /home/s3j/.local/src/celeritas/src/orange/surfaces/detail/SurfaceAction.hh(112): warning: calling a __host__ function("__builtin_unreachable") from a __host__ __device__ function("celeritas::detail::SurfaceAction< ::celeritas_test::CalcSenseDistance> ::operator ()") is not allowed ``` Using `__trap` didn't reduce the number of instructions, but simply omitting the warning works.

amandalund

Awesome @sethrj! Looks great from my end.

src/base/Algorithms.hh

src/orange/surfaces/CylCentered.hh

test/orange/surfaces/CylCentered.test.cc

This is a nonstandard extension that lets us more easily interrogate array sizes.

sethrj · 2021-10-28T11:56:30Z

CI failure is due to disk space issues because ... of course ... vecgeom.

tmdelellis · 2021-10-28T19:43:40Z

Reviewing now!

tmdelellis

Looks good @sethrj

tmdelellis · 2021-10-28T19:48:03Z

src/orange/Types.hh

+ */
+CELER_CONSTEXPR_FUNCTION real_type no_intersection()
+{
+    return numeric_limits<real_type>::infinity();


I may be misreading this, but this will not be able to be called on device.

Sneaky here: numeric_limits is the one in the celeritas namespace. You imply a good point which is that maybe we should change such class names to "resemble" the standard library without actually using the same names: i.e. class NumericLimits rather than class numeric_limits so there's less ambiguity. That can be deferred to another merge but I'm curious of your and the group's thoughts. We already did this with Span and Array, for example, so I'd say we should probably do the same here.

hahnjo · 2022-01-14T11:34:54Z

src/orange/surfaces/GeneralQuadric.hh

+    const real_type x = pos[0];
+    const real_type y = pos[1];
+    const real_type z = pos[2];
+
+    Real3 norm;
+    norm[0] = 2 * a_ * x + d_ * y + f_ * z + g_;
+    norm[1] = 2 * b_ * y + d_ * x + e_ * z + h_;
+    norm[2] = 2 * c_ * z + e_ * y + f_ * x + i_;
+
+    normalize_direction(&norm);
+    return norm;


Hi @sethrj, while looking around in the ORANGE code, I came across this method and how you use it to calculate the safety, ie the smallest distance from a point to the surface. Do you have (public) notes how this is derived mathematically?

I'm wondering about the cases of ellipsoids (or the special case of spheres), cones and cylinders where there are only quadratic terms. If I read the code correctly, it would then produce a null vector for a point at the origin. This likely will give no meaningful answer for the safety, which is trying to intersect a line following the normal vector, will it?

@hahnjo The normal vector at a point on a surface is just the gradient of the surface equation evaluated at that point (x,y,z). I'm not sure what meaning it holds if you evaluate it for a point that's not on the surface (as it appears to be used for the safety distance) so maybe @sethrj can enlighten us both 😄

Yes thank you Paul. The code highlighted here is indeed the gradient of the quadratic expression q(x,y,z), which is the vector normal on the surface but also (modulo the sign) the steepest-descent search direction to solve for a point on the surface q(x,y,z)=0. There are of course points (for ellipsoids), lines (for elliptic cylinders), and I think planes (for hyperbola/parabola/cone) where multiple points are equally close and so components of the gradient go to zero -- so the CalcSafetyDistance algorithm is incomplete for certain degenerate regions.

Anyway, once the steepest descent direction is calculated, the CalcSafetyDistance algorithm solves the quadratic equation that results from evaluating the quadric equation along that direction, resulting in the analytic closest point along that vector.

The comment "I need to check the math on this" on the functor is because this is all rather hand-wavy and I haven't formally evaluated that the nearest surface point is along that gradient... 😬 Nor have I worked out how to calculate the distance for degenerate points. I haven't made safety distance a priority, so this functor is not yet used anywhere: I meant it primarily as an example of how to operate generically on a surface.

One less obvious important point about a complete safety distance algorithm in ORANGE is that the minimum distance to any surface is only the correct safety in the case of a "simple" volume, where crossing any surface exits the volume. In the case of a volume with internal surfaces (e.g. a sphere with a box excluded) the distance to the nearest surface may be far smaller than the distance to the nearest surface crossing that actually exits the volume.

No, I'm fairly convinced this doesn't hold mathematically. I've played with tubes on GeoGebra and it works for a = b = 1. For a = 2 you can already see that the safety sphere intrudes into the tube, and for a = 3 the method doesn't find any intersection at all.

Well, you're definitely right! Sorry for the misleading class and incorrect algorithm. Maybe the safety could be approximated with a few iterations with steepest descent?

Thanks for digging into this, and for the link to the very nifty visualization tool!

I created an issue #352 so we can have further discussions there rather than buried away inside this code review comment 😄

The most promising literature that I could find to date is ON THE DISTANCE FROM A POINT TO A QUADRIC SURFACE. What we need for safety is a sort of "conservative" estimate, ie the returned value can be too low but it must never be too large, I think.

Thanks for digging into this, and for the link to the very nifty visualization tool!

Hehe, I never liked it in high school, but it's one of the few free tools to easily visualize objects and surfaces in 3 dimensions...

What we need for safety is a sort of "conservative" estimate, ie the returned value can be too low but it must never be too large, I think

Exactly. Anything that can give us a conservative estimate (especially if the accuracy increases near the surface itself) would be good.

sethrj added the geometry label Oct 20, 2021

sethrj requested review from paulromano and tmdelellis October 20, 2021 11:55

sethrj added this to In progress in v0.1.0 via automation Oct 20, 2021

sethrj force-pushed the orange branch from 190779e to 2a1fa28 Compare October 21, 2021 01:08

paulromano reviewed Oct 22, 2021

View reviewed changes

src/orange/surfaces/GeneralQuadric.hh Outdated Show resolved Hide resolved

src/orange/Types.hh Outdated Show resolved Hide resolved

sethrj force-pushed the orange branch 2 times, most recently from 1de0101 to 34093d7 Compare October 25, 2021 20:10

sethrj marked this pull request as ready for review October 25, 2021 20:11

sethrj added 10 commits October 26, 2021 06:59

Add types and quadratic solver

746badb

Add CylCentered

fadcf73

Add aligned plane

fda713e

Add general quadric and test placeholder

f721f00

Add stream printing to surfaces

0c2a01f

Add surface inserter, data interface, and surface action

a67045f

Add min_element with indices

76d0fb9

Third-guess myself and change back to iterators

23401c0

Add CUDA surface action test

49c10cb

sethrj force-pushed the orange branch from 5bc629a to 1ad2c64 Compare October 26, 2021 10:59

sethrj added 5 commits October 26, 2021 11:15

fixup! Add surface inserter, data interface, and surface action

dd710dc

fixup! Add aligned plane

f82b255

Document size_ members

d562fdc

Defer unused extra data/type definitions

129b3aa

fixup! Defer unused extra data/type definitions

159f7b6

Use CUDA 10 on emmet and add surface assertion

5d38b71

amandalund approved these changes Oct 28, 2021

View reviewed changes

src/base/Algorithms.hh Outdated Show resolved Hide resolved

src/orange/surfaces/CylCentered.hh Outdated Show resolved Hide resolved

test/orange/surfaces/CylCentered.test.cc Outdated Show resolved Hide resolved

v0.1.0 automation moved this from In progress to Review in progress Oct 28, 2021

sethrj added 4 commits October 28, 2021 07:21

Add sqrt(2) and sqrt(3) constexpr constants

25d54d6

Add extent to Array

3cd051b

This is a nonstandard extension that lets us more easily interrogate array sizes.

Add sphere surface

47dacb5

Apply review feedback

4ff31f1

sethrj added 2 commits October 28, 2021 08:26

Remove unused types

a598fcb

fixup! Add general quadric and test placeholder

d311572

tmdelellis approved these changes Oct 28, 2021

View reviewed changes

sethrj merged commit 4febdeb into celeritas-project:master Oct 28, 2021

v0.1.0 automation moved this from Review in progress to Done Oct 28, 2021

sethrj deleted the orange branch October 28, 2021 21:58

sethrj mentioned this pull request Jan 4, 2022

Implement portable in-house geometry #267

Closed

9 tasks

hahnjo reviewed Jan 14, 2022

View reviewed changes

sethrj mentioned this pull request Jan 20, 2022

Add safety distance calculation #990

Open

sethrj added orange Work on ORANGE geometry engine enhancement New feature or request and removed geometry labels Feb 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial portable implementation of ORANGE: oak ridge geometry engine #291

Initial portable implementation of ORANGE: oak ridge geometry engine #291

sethrj commented Oct 20, 2021

paulromano commented Oct 20, 2021

sethrj commented Oct 20, 2021

sethrj commented Oct 21, 2021

sethrj commented Oct 21, 2021

sethrj commented Oct 25, 2021

amandalund left a comment

sethrj commented Oct 28, 2021

tmdelellis commented Oct 28, 2021

tmdelellis left a comment

tmdelellis Oct 28, 2021

sethrj Oct 28, 2021

hahnjo Jan 14, 2022

paulromano Jan 14, 2022

sethrj Jan 14, 2022

hahnjo Jan 20, 2022

sethrj Jan 20, 2022

sethrj Jan 20, 2022

hahnjo Jan 20, 2022

sethrj Jan 20, 2022

Initial portable implementation of ORANGE: oak ridge geometry engine #291

Initial portable implementation of ORANGE: oak ridge geometry engine #291

Conversation

sethrj commented Oct 20, 2021

paulromano commented Oct 20, 2021

sethrj commented Oct 20, 2021

sethrj commented Oct 21, 2021

sethrj commented Oct 21, 2021

sethrj commented Oct 25, 2021

amandalund left a comment

Choose a reason for hiding this comment

sethrj commented Oct 28, 2021

tmdelellis commented Oct 28, 2021

tmdelellis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment