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.

Sign up for GitHub

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

Jump to bottom

Update tracking to use Acts v36.0 #1454

Merged
merged 54 commits into from
Sep 16, 2024
Merged

Update tracking to use Acts v36.0 #1454

merged 54 commits into from
Sep 16, 2024

Conversation

bloodyyugo
Copy link
Contributor

Pull request addresses issue 1356, upgrading tracking to use ACTs version 36.0.

The development branch is actsv36.0 and compiles, builds and runs. Comparisons between tracking using the previous ACTs version (19.X ... something) are shown on the issue page linked above.

There is one outstanding problem in that we see fewer hits-on-track for tagger tracks with this update. We don't see this in the recoil for some reason. We are investigating but this issue can be closed without it.

bloodyyugo and others added 23 commits August 14, 2024 07:48
@bloodyyugo
move acts to v36 and required ldmx::tracking code changes, ldmx-sw bu…
f1ccef8 
…ilds but does not link yet
@tomeichlersmith
remove now non-existent acts component from linking
85cfe4e 
The ActsPluginIdentification does not exist within ACTS v36 and so
asking for it to be built just gets ignored since its not an option ACTS
looks for; however, asking to link to it errors since this component is
not a valid library that can be linked to (it is never built).

We can remove reference to this plugin and point out that the
default-build of acts appears to satisfy our needs.
@bloodyyugo
v36 is building, linking, running and give resonable results. Still n…
356047a 
…eed to update GSF track fitting and restore a few features.
@bloodyyugo
forgot a couple of changes to MeasurementCalibrator
c6ce7b8
@bloodyyugo
fix up CKF so that it makes everything for ldmx::track; GSF builds bu…
08fb5ad 
…t not sure if working yet
@bloodyyugo
Once in a while CKFProcessor crashes when it cant extrapolate to targ…
19e4e54 
…et state. Put in a catch and some info statements
@bloodyyugo
Add back a block I accidently deleted
7a1a2ac
@EBerzin
adapted GSF for v36 and added tagger/recoil flexibility
8a77993
@bloodyyugo @tvami
move acts to v36 and required ldmx::tracking code changes, ldmx-sw bu…
3688575 
…ilds but does not link yet
@tomeichlersmith @tvami
remove now non-existent acts component from linking
181a274 
The ActsPluginIdentification does not exist within ACTS v36 and so
asking for it to be built just gets ignored since its not an option ACTS
looks for; however, asking to link to it errors since this component is
not a valid library that can be linked to (it is never built).

We can remove reference to this plugin and point out that the
default-build of acts appears to satisfy our needs.
@bloodyyugo @tvami
v36 is building, linking, running and give resonable results. Still n…
7eb0f96 
…eed to update GSF track fitting and restore a few features.
@bloodyyugo @tvami
forgot a couple of changes to MeasurementCalibrator
c30bd8c
@bloodyyugo @tvami
fix up CKF so that it makes everything for ldmx::track; GSF builds bu…
ded9895 
…t not sure if working yet
@bloodyyugo @tvami
Once in a while CKFProcessor crashes when it cant extrapolate to targ…
650968f 
…et state. Put in a catch and some info statements
@bloodyyugo @tvami
Add back a block I accidently deleted
8aa69a3
@EBerzin @tvami
adapted GSF for v36 and added tagger/recoil flexibility
d0af2ab
@tvami
Merge branch 'actsv36.0' of github.com:LDMX-Software/ldmx-sw into act…
b38ac17 
…sv36.0
@bloodyyugo
add new plots and ranges to dqm
0b0e445
@bloodyyugo
Merge branch 'trunk' into actsv36.0
5e098b0
@bloodyyugo
update the example reco so that it actually works right
273906a
@bloodyyugo
clang_format and some other cleanup
89b61e0
@bloodyyugo
Merge branch 'actsv36.0' of github.com:LDMX-Software/ldmx-sw into act…
2cb03b6 
…sv36.0
@bloodyyugo
Merge branch 'trunk' into actsv36.0
9d3b6b1
@tvami
Copy link
Member

tvami commented Sep 13, 2024

hey @bloodyyugo we'll have to rebase this due to the fact that my other PR was touching on the same files

@bloodyyugo
Copy link
Contributor Author

Yeah, I'll redo now.

bloodyyugo and others added 16 commits September 14, 2024 11:29
@bloodyyugo @tvami
Update Tracking/include/Tracking/Sim/MeasurementCalibrator.h
1e5bed4 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/include/Tracking/Sim/MeasurementCalibrator.h
974c04d 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/include/Tracking/Sim/MeasurementCalibrator.h
f28dcb7 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/src/Tracking/Reco/CKFProcessor.cxx
8a8d2cd 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/src/Tracking/Reco/SeedFinderProcessor.cxx
a764456 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/include/Tracking/Reco/TrackExtrapolatorTool.h
3f722a4 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/include/Tracking/Reco/TrackExtrapolatorTool.h
c0fa81c 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/include/Tracking/Reco/TrackExtrapolatorTool.h
571af65 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/include/Tracking/Reco/TrackExtrapolatorTool.h
5c4113b 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/include/Tracking/Reco/TrackExtrapolatorTool.h
776dbbd 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/src/Tracking/Reco/Vertexer.cxx
7a9ff1d 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/src/Tracking/Reco/VertexProcessor.cxx
358e15a 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/src/Tracking/Reco/Vertexer.cxx
0b4de1f 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/src/Tracking/Reco/CKFProcessor.cxx
cdf353c 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/src/Tracking/Reco/CKFProcessor.cxx
31c7fdd 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo @tvami
Update Tracking/src/Tracking/Reco/CKFProcessor.cxx
376d912 
Co-authored-by: Tamas Vami <[email protected]>
@bloodyyugo bloodyyugo self-assigned this Sep 14, 2024
@bloodyyugo
Copy link
Contributor Author

@tvami I think I resolved all of the issues

@tvami
Copy link
Member

tvami commented Sep 15, 2024

Thanks Matt! I ran it on the ecal_pn + tracking config and I get the following error

Unrecognized Exception: vector::_M_range_check: __n (which is 16) >= this->size() (which is 14)

I thought this might be because of the default now in tracking geo is the no-cal geom.
So I added

from LDMX.Tracking.geo import TrackersTrackingGeometryProvider as trackgeo
trackgeo.get_instance().setDetector('ldmx-det-v14-8gev')

to the
https://github.com/LDMX-Software/ldmx-sw/blob/trunk/.github/validation_samples/ecal_pn/config.py
and ran that. It leads to the same exception.

This could be connected to the loss of hits in the tagger, given that the 14 sounds like the number of tagger layers. OTOH I dont know what the 16 is coming from. Anyway, I confirm that the reco.py works out of the box, but we should make sure it works with any geometry (that contains the trackers)

@tvami
Copy link
Member

tvami commented Sep 15, 2024

Nvm, I found the source of it, it's the fact that measurement_collection has to be passed in the config, so adding

recoil_dqm.measurement_collection=digiRecoil.out_collection

fixes it. But this means the CI fails with the current config, so @bloodyyugo can you please propagate the changes in the reco.py to .github/validation_samples/ecal_pn/config.py as well? Thanks!

tvami
tvami approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@tvami tvami left a comment

Choose a reason for hiding this comment

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

The only thing left is the CI config change, but we could also do that in a next PR.

@bloodyyugo bloodyyugo merged commit 77fffe1 into trunk Sep 16, 2024
2 checks passed
@bloodyyugo bloodyyugo deleted the actsv36.0 branch September 16, 2024 21:48
@tomeichlersmith tomeichlersmith mentioned this pull request Sep 17, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@EBerzin EBerzin EBerzin approved these changes

@tvami tvami tvami approved these changes

@pbutti pbutti Awaiting requested review from pbutti

Assignees

@bloodyyugo bloodyyugo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change ACTS to v36
4 participants
@bloodyyugo @tvami @EBerzin @tomeichlersmith