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

Bug: last material interaction missing in validation output? #1882

Closed
noemina opened this issue Feb 21, 2023 · 9 comments · Fixed by #1968
Closed

Bug: last material interaction missing in validation output? #1882

noemina opened this issue Feb 21, 2023 · 9 comments · Fixed by #1968

Comments

@noemina
Copy link
Contributor

noemina commented Feb 21, 2023

While mapping the material on the ITk tracking geometry (using the standalone workflow), I have seen that the material interaction for the outer boundary seems not to be collected in the root file.
Things I have already tested:

  • I checked that the mapping procedure hit the boundary layers I am selecting in the json file (I see the material indeed appearing in the mapping file)
  • I also run the navigation in verbose mode to check that the same boundaries I map the material onto are hit

I start believing that the last material interaction is simply not saved in the output file of the material validation job?

@Corentin-Allaire, could you help me with this?

@noemina
Copy link
Contributor Author

noemina commented Feb 21, 2023

An outer boundary in expected all around the tracking geometry...
material_comparison

@Corentin-Allaire
Copy link
Contributor

I can have a look at the writing in the validation tomorrow. I will let you know. But we agree it only affect the validation and not the performance right ?

@Corentin-Allaire
Copy link
Contributor

So looking at MaterialInteractor I notice the following lines (61-64) :

    // If we are on target, everything should have been done
    if (state.navigation.targetReached) {
      return;
    }

It seem we do not perform the material interaction on the target surface. In a way this makes sense as it doesn't matter for the propagation but to get proper validation plots maybe we should change it to :

   // If we are on target, everything should have been done
   if (state.navigation.targetReached && !recordInteractions) {
     return;
   }

That way we would record material even on the target surface.
Let me know if that fixes your issue.

@asalzburger
Copy link
Contributor

In fact, we should make the 'SurfaceMaterial` split factor decide if the material interaction on the target surface should happen or not, right?

@Corentin-Allaire
Copy link
Contributor

@asalzburger I am pretty sure that the split factor even if it exit is not used anywhere right now. But if we where to properly implement it everywhere you are right that it would make sense

@noemina
Copy link
Contributor Author

noemina commented Feb 24, 2023

Hello @Corentin-Allaire , I have tested your suggestion this morning, but unfortunately the last material interaction is still missing. :(

@Corentin-Allaire
Copy link
Contributor

Hi @noemina, so I am a bit at a loss at this point. The propagator action should happen before the aborter so I don't understand why we don't go through the MaterialInteractor... If you have the time at some point could you add a cout statement to the MaterialInteractor (for example one that print the surface ID) just to be sure if the issue is that we don't record the material or if it is that we don't write it to file properly ?

@Corentin-Allaire
Copy link
Contributor

Never mind my last message, we don't do the interaction because else we would print the energy loss and we don't.

@noemina
Copy link
Contributor Author

noemina commented Mar 20, 2023

After some investigation, I found the reason of this issue. It is now fixed.

material_comparison

@kodiakhq kodiakhq bot closed this as completed in #1968 Apr 4, 2023
kodiakhq bot pushed a commit that referenced this issue Apr 4, 2023
This PR fixes #1882.

The reason why the outer boundary was not seen is due to the accumulation of invalid material on it. Skipping un-physical steps (step length = 0) restores a sane behaviour of the mapping procedure.

I have also added a check on existence of the `event_id` branch in the `TChain`. If the branch doesn't exist, the batch size is evaluated to be 1.

@Corentin-Allaire @goetzgaycken
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants