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

Refactor the event map #692

Merged
merged 8 commits into from
Oct 18, 2024
Merged

Conversation

beomki-yeo
Copy link
Contributor

@beomki-yeo beomki-yeo commented Aug 31, 2024

Outdated description

I will go through a massive cleanup in the near future, but let me mess it up one more time..
This PR also removes the annoying clusterization in the event map

UPDATE

I compared the local positions of measurements from traccc CPU CCA against the local positions of measurements.csv (data directory: odd/geant4_1muon_100GeV).
And I found that they have the same local positions (the below table shows the measurements of the first event).
I thought the local positions of measurements.csv are the truth value, so there must be gap between them.

From CCA From measurements.csv
(-6.775,32.735) (-6.775,32.735)
(-5.825,9.025) (-5.825,9.025)
(-5.075,-10.7355) (-5.075,-10.7355)
(-1.325,-28.7803) (-1.325,-28.7803)
(3.825,2.49362) (3.825,2.49362)
(6.475,24.1524) (6.475,24.1524)
(-6.42745,25.8361) (-6.42745,25.8361)
(-3.075,5.45458) (-3.075,5.45458)
(-4.725,6.13291) (-4.725,6.13291)
(7.875,7.82025) (7.875,7.82025)
(-5.60696,5.13736) (-5.60696,5.13736)
(0.125,-9.88765) (0.125,-9.88766)
(4.13824,-30.5525) (4.13824,-30.5525)
(6.21763,-5.97566) (6.21763,-5.97566)
(-7.73042,-9.33519) (-7.73042,-9.33519)
(1.825,-33.641) (1.825,-33.641)
(2.31866,33.1868) (2.31866,33.1868)
(2.625,25.8589) (2.625,25.8589)
(-0.825,-4.29537) (-0.825,-4.29537)
(-6.825,-34.2) (-6.825,-34.2)
(-32.325,40.2) (-32.325,40.2)
(-28.125,-29.4) (-28.125,-29.4)
(-4.125,52.2) (-4.125,52.2)
(-3.375,-7.8) (-3.375,-7.8)
(17.475,-23.4) (17.475,-23.4)
(-2.775,-57) (-2.775,-57)
(3.6,-34.2) (3.6,-34.2)
(12.225,28.8) (12.225,28.8)
(10.2,-21) (10.2,-21)
(-11.475,-25.8) (-11.475,-25.8)
(-15.975,-49.8) (-15.975,-49.8)
(0.450002,0.600002) (0.450001,0.600002)
(-10.35,-25.8) (-10.35,-25.8)
(6.675,-47.4) (6.675,-47.4)
(7.575,17.4) (7.575,17.4)
(-3.575,51.8) (-3.575,51.8)
(-2.325,-31.2) (-2.325,-31.2)
(-5.025,29.4) (-5.025,29.4)
(17.25,0) (17.25,0)
(15.25,0) (15.25,0)
(14.75,0) (14.75,0)
(12.75,0) (12.75,0)
(-6.75,0) (-6.75,0)
(-6.125,0) (-6.125,0)
(34.125,0) (34.125,0)
(34.125,0) (34.125,0)
(42.875,0) (42.875,0)
(43,0) (43,0)
(-38.375,0) (-38.375,0)
(46,0) (46,0)
(42.625,0) (42.625,0)
(-37.875,0) (-37.875,0)
(-35.375,0) (-35.375,0)

Perhaps, measurements.csv is not for storing the truth value but for storing CCA value in ACTS simulation? (But how does ACTS simulation perform the CCA?) Maybe I am just doing something stupid. @asalzburger @krasznaa Do you have some ideas on this?

PR Description

I decided to refactor the event_map along with the measurement creation validation as I found it makes things easier. Followings are the major changes:

  1. event_map and event_map are removed and unified into event_data which can support both acts simulation data and detray simulation data.

  2. Validation suites for the measurement creation are added. As expected, the qualities of the pull values of d0 and z0 are not great, thus, the conditions are quite loose.

  3. csv geometry is deprecated as we are not supposed to use it anymore. Also the the ACTS cell data format has replaced hit_id column (csv tml) with measurement_id column (new format), and it is quite painful to deal with different data formats.

Physics Performance of seq_example (double precision)

The pull value from the measurement creation is not great but can be tolerated for the moment.
The pull value from Kalman fit is definitely bad for unknown reasons - It will be followed up in the near future

./bin/traccc_seq_example --detector-file=geometries/odd/odd-detray_geometry_detray.json --digitization-file=geometries/odd/odd-digi-geometric-config.json --grid-file=geometries/odd/odd-detray_surface_grids_detray.json --input-directory=odd/geant4_10muon_100GeV --check-performance --input-events=50

image
image
image

Physics Performance of seeding_example (double precision)

./bin/traccc_seeding_example --detector-file=geometries/odd/odd-detray_geometry_detray.json --digitization-file=geometries/odd/odd-digi-geometric-config.json --grid-file=geometries/odd/odd-detray_surface_grids_detray.json --input-directory=odd/geant4_10muon_100GeV --check-performance --input-events=50

image
image

@beomki-yeo beomki-yeo marked this pull request as draft August 31, 2024 12:32
@beomki-yeo
Copy link
Contributor Author

Indeed DigitizationAlgorithm of ACTS write measurements.csv with the same measurement creation algorithm. I will use the truth hit information and do the coordinate transformation instead

@stephenswat
Copy link
Member

I believe that this is due to the fact that ACTS digitization is a rather simple heuristic that does not have any uncertainty, and that also ensures that the weighted centroid of the hits matches exactly with the measurement; i.e., the traccc CCA is the perfect inverse function of the ACTS digitization.

@beomki-yeo
Copy link
Contributor Author

the traccc CCA is the perfect inverse function of the ACTS digitization.

Are you sure about this? The digitization seems more complicated than that 🤔

@beomki-yeo
Copy link
Contributor Author

beomki-yeo commented Sep 2, 2024

@beomki-yeo beomki-yeo marked this pull request as ready for review September 10, 2024 09:48
@beomki-yeo beomki-yeo marked this pull request as draft September 10, 2024 11:37
@beomki-yeo beomki-yeo force-pushed the validate-meas-creation branch 2 times, most recently from e5f09ad to c4d16f1 Compare September 10, 2024 21:41
@beomki-yeo beomki-yeo marked this pull request as ready for review September 10, 2024 21:41
@beomki-yeo beomki-yeo force-pushed the validate-meas-creation branch from c4d16f1 to cec247d Compare September 10, 2024 21:42
@beomki-yeo beomki-yeo changed the title WIP: Add validation for measurement creation Refactor the event map Sep 10, 2024
@beomki-yeo beomki-yeo force-pushed the validate-meas-creation branch from cec247d to cc07ad9 Compare September 10, 2024 22:02
@beomki-yeo beomki-yeo force-pushed the validate-meas-creation branch from cc07ad9 to 9fba431 Compare September 10, 2024 22:40
@beomki-yeo beomki-yeo force-pushed the validate-meas-creation branch 2 times, most recently from a861e38 to 53a83ee Compare September 11, 2024 13:52
@beomki-yeo
Copy link
Contributor Author

The PR won't be able to pass the test unless the EOS data is updated.
New data file is at: https://cernbox.cern.ch/s/Wyr4TKwFpENhO9H

@beomki-yeo beomki-yeo force-pushed the validate-meas-creation branch from 53a83ee to 3aae79b Compare September 11, 2024 17:03
@beomki-yeo beomki-yeo marked this pull request as draft September 26, 2024 13:30
@beomki-yeo beomki-yeo force-pushed the validate-meas-creation branch 4 times, most recently from 5fea9ce to 1503e97 Compare September 26, 2024 23:39
@beomki-yeo beomki-yeo force-pushed the validate-meas-creation branch from e0e1a50 to 97cfe67 Compare September 27, 2024 00:41
@beomki-yeo beomki-yeo marked this pull request as draft September 27, 2024 00:46
@beomki-yeo beomki-yeo marked this pull request as ready for review September 27, 2024 01:11
@beomki-yeo
Copy link
Contributor Author

beomki-yeo commented Sep 27, 2024

@krasznaa @stephenswat

I removed the event maps and replace them with the event_data.
What it does is more or less the same.. hopefully more readable and makes sense.

Especially the measurement information is handled in a more sophisticated way - there are three types of them in event_data

  1. meas: from measurements.csv of detray/acts simulation. It should be noted that they are not the truth measurements. For example, measurements.csv of acts simulation is a result of measurement_creation on the truth clusters, which means they have some uncertainties from the algorithm.

  2. truth_meas: global-to-local transformation of hits.csv, so they are truth information

  3. found_meas: Similar to meas, but results of measurement_creation on the found clusters (from CCL algorithms). Thus, it is less accurate than meas in case of clusters are from multiple particles. They are not filled during the construction of event_data. Users need to fill out them later after completing the CCA.

I have not checked the outputs of --check_performance as the current example executables are not stable but there will be follow-up PRs soon to fix this.

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I like that you make use of the detector description object. 😄 Though I don't fully understand how... Could you explain a bit more how use_detray_simulation is meant to be used/understood exactly?

@beomki-yeo
Copy link
Contributor Author

Based on #729 #733 #734 #735

@beomki-yeo beomki-yeo force-pushed the validate-meas-creation branch from fa07bf4 to 9a239f7 Compare October 15, 2024 05:06
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

On one hand it would be nice to get all of this into main before CHEP, but I feel relatively strongly about some of the design points that I highlighted. So I'd still rather spend the time to set this up correctly, for our future selves.

@krasznaa
Copy link
Member

Beomki, see me at one point today, as we need to talk this out live.

  • The application needs to have an additional boolean flag;
  • The I/O code doesn't.

I'll explain in more detail face to face.

@beomki-yeo beomki-yeo requested a review from krasznaa October 16, 2024 14:47
@beomki-yeo
Copy link
Contributor Author

It is good to go from my side

@beomki-yeo beomki-yeo force-pushed the validate-meas-creation branch from f253944 to 96bef6b Compare October 16, 2024 19:05
@beomki-yeo
Copy link
Contributor Author

@krasznaa The IO input is fixed now.
I don't know why build CI remains unfinished but I think that can be ignored

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

I'm fine with the I/O code's general setup at this point. The main part of the code, the event_data class, I can also make my peace with. (The goal sanctifies the means...) Let's just fix up some of the smaller issues, and then finally push this in today.

Copy link

@beomki-yeo beomki-yeo requested a review from krasznaa October 18, 2024 11:21
Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Let's get this thing finally in. 👍

@krasznaa krasznaa enabled auto-merge (squash) October 18, 2024 11:45
@krasznaa krasznaa merged commit a10658a into acts-project:main Oct 18, 2024
19 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants