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

fix: Skipping un-physical steps in material mapping (#1882) #1968

Merged

Conversation

noemina
Copy link
Contributor

@noemina noemina commented Mar 20, 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

@noemina noemina self-assigned this Mar 20, 2023
@noemina noemina added the Bug Something isn't working label Mar 20, 2023
@noemina noemina added this to the next milestone Mar 20, 2023
Copy link
Contributor

@Corentin-Allaire Corentin-Allaire left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a lot for solving this issue !

@codecov
Copy link

codecov bot commented Mar 20, 2023

Codecov Report

Merging #1968 (6240193) into main (1316c69) will not change coverage.
The diff coverage is n/a.

❗ Current head 6240193 differs from pull request most recent head 934306f. Consider uploading reports for the commit 934306f to get more accurate results

@@           Coverage Diff           @@
##             main    #1968   +/-   ##
=======================================
  Coverage   49.83%   49.83%           
=======================================
  Files         415      415           
  Lines       23593    23593           
  Branches    10681    10681           
=======================================
  Hits        11758    11758           
  Misses       4330     4330           
  Partials     7505     7505           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

github-actions bot commented Mar 20, 2023

📊 Physics performance monitoring for 934306f

Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@noemina noemina force-pushed the fix-bug-material-map-unphysical-steps branch from cba1ebf to df87f7a Compare March 21, 2023 14:23
@noemina
Copy link
Contributor Author

noemina commented Mar 21, 2023

@Corentin-Allaire , do you understand why the tests fail?

@Corentin-Allaire
Copy link
Contributor

Corentin-Allaire commented Mar 21, 2023

@Corentin-Allaire , do you understand why the tests fail?

Yes now that you have fixed the issue in the material mapping the map created in the test are different from the on in the reference.
You need to update the /Examples/Python/tests/root_file_hashes.txt with the new hashes that are in the CI log.

test_material_mapping__material-map_tracks.root: 9f87483d82672acb33f238debe33bdded4233de2341ba322a485e6cc6161eaf7
test_material_mapping__propagation-material.root: c0748d651adcc155cf894dee93e8aa458faad4ddcb6b757546cdcefc8cfb89e8
test_volume_material_mapping__material-map-volume_tracks.root: f6fb7a8babbf8d48f48a147af32e8ae3dac58ea19bae265f4afecccf63ff8a45
test_volume_material_mapping__propagation-volume-material.root: 65479aaa8c6d58cd3a1dbc64a1fae62247a2150c51083cb268d06b740caa35b4

@noemina
Copy link
Contributor Author

noemina commented Mar 21, 2023

@Corentin-Allaire , do you understand why the tests fail?

Yes now that you have fixed the issue in the material mapping the map created in the test are different from the on in the reference. You need to update the /Examples/Python/tests/root_file_hashes.txt with the new hashes that are in the CI log.

Yes, Thanks. My question was if we expect the tests to fail. This PR skips empty steps, so we did have empty steps in the input file used in the tests, which were messing up the results there as well?

@Corentin-Allaire
Copy link
Contributor

Corentin-Allaire commented Mar 21, 2023

@Corentin-Allaire , do you understand why the tests fail?

Yes now that you have fixed the issue in the material mapping the map created in the test are different from the on in the reference. You need to update the /Examples/Python/tests/root_file_hashes.txt with the new hashes that are in the CI log.

Yes, Thanks. My question was if we expect the tests to fail. This PR skips empty steps, so we did have empty steps in the input file used in the tests, which were messing up the results there as well?

If I understood you MR correctly those empty step appear a the end of the detector so we probably have some in all of our files.
It might not be a bad idea, at some point, to map the ODD again and check if your PR makes a big difference

@noemina noemina force-pushed the fix-bug-material-map-unphysical-steps branch 2 times, most recently from 5a615fd to ec4e70b Compare April 4, 2023 11:17
@noemina noemina force-pushed the fix-bug-material-map-unphysical-steps branch from ec4e70b to 6240193 Compare April 4, 2023 12:57
@kodiakhq kodiakhq bot merged commit 921da6d into acts-project:main Apr 4, 2023
36 checks passed
@github-actions github-actions bot removed the automerge label Apr 4, 2023
@paulgessinger paulgessinger modified the milestones: next, v25.0.0 Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: last material interaction missing in validation output?
3 participants