Search class to wrap internal data #144

Fuad-HH · 2025-02-27T03:21:26Z

As a continuation of standardizing the search method (next steps of #143), this wraps the search related particle state arrays inside this class. It includes the following tasks:

Create ParticleTracer class
Write test case
Add consequent search in the test case
remove finishedUnmoved
Move particle_done out as a parameter
Make maxLoop a variable in ParticleTracer class
Add a test case with GMSH mesh to test node orientation check or correction
Rename trace_particle_through_mesh function parameters
Clean up: hardcoded parameters, comments, etc.

- passing all tests except smoke_test_particle It is giving a warning for MPI oversubscribing and "Structure not initalized at Particle"

- passing all tests

instead of checking if tol was passed using negative values, it is made optional. In the future, tol will be calculated outside according to the standardization plan and for this reason, we don't need it to be passed as reference or pointer

it is passed as a parameter rather than computing it every time the search is called.

it's an initial step to make the function standard with proper docuemntation. [important] there are some breaking changes and the function is under heavy development

the functor decides what happens to the particles at the boundary

next_element parameter holds the next element when a particle intersects a face. This is added to both the trace_particle_through_mesh and the functor. The functor may or may not use the value, but there's a high chance to be used. [Important] The test is failing. This is because although the particles doesn't intersect face 0 (it is along the way of the ray going forward) the closeness algorithm finds it as the intersected face.

- works for particles going to a new element - or remains in the same - [to be fixed] particles leaking out

next_element becomes -1 when particle leaks

fix search_mesh in accordance with changes in trace_particle_through_mesh [future fixes] - interPoints should give the face intersection location when leaking out instead of the destination

- same test case repurposed from search_mesh test case

Fuad-HH · 2025-02-27T03:24:47Z

@jacobmerson, Could you please take a look? Does the class look like you suggested?

- it creates confusion without solving anything significant - tested

- it should save the allocation time for every call of search

jacobmerson · 2025-02-28T10:36:17Z

src/ParticleTracer.tpp

+    Omega_h::Write<Omega_h::LO> GetElemIds() { return elem_ids_; }
+
+    Omega_h::Write<Omega_h::LO> GetInterFaces() { return inter_faces_; }
+
+    Omega_h::Write<Omega_h::Real> GetInterPoints() { return inter_points_; }
+
+    Omega_h::Write<Omega_h::LO> GetLastExits() { return last_exits_; }


Couple comments on this:

Please make any words fully spelled out as it is easier to understand. i.e. GetElemIds -> GetElementIds, GetInterFaces -> GetIntersectionFaces, etc.

The second question is more philosophical. Why should these be in the public interface? Do we expect the user of this class to need to modify this?

Should be marked [[nodiscard]] and const and should not return Write instead, should return a Read, or better a span<const T>

I have changed them to Read. It makes more sense here since we don't want the user to change the internal state of the particles. However, I think there can be cases where the user may want to know the states after the search, for example, through which face particles leaked. It can be argued that these can be done inside the functor, but it will provide an easier option to read them whereas the functor can be difficult to understand for a new user of this class.

jacobmerson · 2025-02-28T10:44:26Z

src/ParticleTracer.tpp

+        auto particle_orig = ptcls_->template get<0>();
+        auto particle_dest = ptcls_->template get<1>();
+        auto particle_ids = ptcls_->template get<2>();


We should provide indirection here so the user can decide which IDs correspond to the locations. This will also make it easier if we try to move the other particle arrays into the particle datastructure.

The way to do this is as follows:

Create a template struct called ParticleDataStructureIndexMap we may change name later. This should take a single template argument which will be the particle datastructure.

Overload the map for your particle datastructure with constexpr int members called OriginalPosition, DestinationPosition, ParticleID. For your default particle datastructure they will be set to 0, 1, 2.

Have your class take an additional template argument which will be this map. The default type should be ParticleDataStructureIndexMap<ParticleType>

Do you mean something like this?

template<typename ParticleType> struct ParticleDataStructureIndexMap { static constexpr int OriginalPosition = 0; static constexpr int DestinationPosition = 1; static constexpr int ParticleID = 2; }; template<typename ParticleType, typename Func, typename ParticleIDMap = ParticleDataStructureIndexMap<ParticleType>> class ParticleTracer { ...

If this is what you mean,

Why does the struct need the ParticleType template?

Can't it be just an enum class?

If a user wants to specify the map themselves, they have to specify all three of them which can be difficult.

Yes, this is what I mean. This extra indirection let's us decouple the position of the information in the particle struct from the the items. I.e., if the user wants to put the location in different positions they can easily do that.

I'm not 100% sure that we want to include the template on the particle structure since you could have the same particle structure type with different semantic meaning. But the goal there is that the compiler will "intelligently" pick the right index based on the particle structure the user passes in.

jacobmerson · 2025-02-28T10:45:07Z

src/ParticleTracer.tpp

+    Omega_h::Write<Omega_h::LO> ptcl_done_;
+
+
+    bool validate_internal_data_sizes() {


what is this function doing? Needs a comment that describes what this check does.

jacobmerson · 2025-02-28T10:48:02Z

src/ParticleTracer.tpp

+        return true;
+    }
+
+    void reset_particle_done() {


The name done is still a bit confusing to me. What about particle_requires_tracing or particle_at_final_location?

jacobmerson · 2025-02-28T10:48:18Z

src/pumipic_adjacency.tpp

+            //o::Real quality = -1;
+            //o::LO bestFace = -1;


remove commented code.

jacobmerson · 2025-02-28T10:48:37Z

src/pumipic_adjacency.tpp

+              //if (dproj > -tol && (quality < 0 || closeness < quality) && lastExit[ptcl] == -1) {
+              //  quality = closeness;
+              //  bestFace = face_id;
+              //  xPoints[3*ptcl] = xpts[0]; xPoints[3*ptcl + 1] = xpts[1]; xPoints[3*ptcl + 2] = xpts[2];
+              //}


remove commented code

jacobmerson · 2025-02-28T10:48:52Z

src/pumipic_adjacency.tpp

+            //if (lastExit[ptcl] == -1) {
+            //    printf("!!!!!!! Best face %d\n", bestFace);
+            //    lastExit[ptcl] = bestFace;
+            //}


remove commented code.

jacobmerson · 2025-02-28T10:49:09Z

src/pumipic_adjacency.tpp

+                xPoints[3*ptcl+2] = x_ps_tgt(ptcl,2);
            }
-            ptcl_done[ptcl] = (lastExit[ptcl] == -1);
+            //ptcl_done[ptcl] = (lastExit[ptcl] == -1);


remove commented code

jacobmerson · 2025-02-28T10:52:25Z

src/pumipic_adjacency.tpp

-        }
-        else {
-          elem_ids[pid] = exposed ? -1 : elem_ids[pid]; //leaves domain if exposed
+    auto checkIntersectionwGeomBoundary = PS_LAMBDA(const int e, const int pid, const int mask) {


Rename this to intersectsWithGeometryInterface when you write functions that will return a bool (or array of bool) put a question mark after the name in your head. If the answer to that question matches the result of your boolean then it will be clear what the function is doing.

With the current name, if you say "check the intersection with the geometry boundary?` the name does not give any clues about what true means and what false means.

- added update particle position function for the class

- constructor gets picparts instead of the mesh - rebuild: update particle positions and migrate (based on the passed flag)

- major bug - Related test cases are fixed too

Fuad-HH added 11 commits February 11, 2025 15:13

calculate tol for search if given is not valid/negative

4616e05

- passing all tests except smoke_test_particle It is giving a warning for MPI oversubscribing and "Structure not initalized at Particle"

pass tol as reference

05c485d

- passing all tests

made tol optional

996c9a3

instead of checking if tol was passed using negative values, it is made optional. In the future, tol will be calculated outside according to the standardization plan and for this reason, we don't need it to be passed as reference or pointer

make elmArea a parameter

025c757

it is passed as a parameter rather than computing it every time the search is called.

define trace_particle_through mesh function behavior

3aadb45

it's an initial step to make the function standard with proper docuemntation. [important] there are some breaking changes and the function is under heavy development

moved set_new_element to functor

8d56edc

the functor decides what happens to the particles at the boundary

fix find exit face and parameter next_element

ccd7cd1

- works for particles going to a new element - or remains in the same - [to be fixed] particles leaking out

fix find_exit_face and next_element when leaks

622ed6e

next_element becomes -1 when particle leaks

fix search_mesh

c88c8cc

fix search_mesh in accordance with changes in trace_particle_through_mesh [future fixes] - interPoints should give the face intersection location when leaking out instead of the destination

added ParticleTracer class a test case

5d76b76

- same test case repurposed from search_mesh test case

Fuad-HH added 2 commits February 27, 2025 12:55

remove finishUnmoved

8d2de22

- it creates confusion without solving anything significant - tested

move ptcl_done parameter out

adc993d

- it should save the allocation time for every call of search

jacobmerson reviewed Feb 28, 2025

View reviewed changes

jacobmerson mentioned this pull request Feb 28, 2025

Standardize trace_particle_through_mesh #143

Open

6 tasks

Fuad-HH added 4 commits February 28, 2025 14:41

add consequnt search for the test

3e3042e

- added update particle position function for the class

made the returned internal states read-only

470feeb

rebuild function

3eaceb0

- constructor gets picparts instead of the mesh - rebuild: update particle positions and migrate (based on the passed flag)

fix typo in find_exit_face

77113e3

- major bug - Related test cases are fixed too

		Omega_h::Write<Omega_h::LO> ptcl_done_;


		bool validate_internal_data_sizes() {

Search class to wrap internal data #144

Are you sure you want to change the base?

Search class to wrap internal data #144

Uh oh!

Conversation

Fuad-HH commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fuad-HH commented Feb 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fuad-HH commented Feb 27, 2025 •

edited

Loading