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: convert TrackParamTruthInit to use DD4hep units #1198

Merged

Conversation

wdconinc
Copy link
Contributor

@wdconinc wdconinc commented Jan 2, 2024

Briefly, what does this PR introduce?

Units are set (via command line or config files) assuming a certain unit system. We have chosen DD4hep for this in all other cases (in part because it comes with a parser). Having one algorithm use Acts units is potentially confusing since distances may be passed in the wrong units.

This PR converts TrackParamTruthInit to store the config variables in DD4hep units, and modifies the algorithm itself to interpret the config in DD4hep units. No changes are expected to the output.

No values are specified in the factory or in the plugin, so this is the only change that should be needed.

What kind of change does this PR introduce?

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

No.

Does this PR change default behavior?

No.

@wdconinc wdconinc linked an issue Jan 2, 2024 that may be closed by this pull request
@github-actions github-actions bot added the topic: tracking Relates to tracking reconstruction label Jan 2, 2024
@wdconinc wdconinc force-pushed the 1166-trackparamtruthinitconfig-assumes-acts-unit-system branch from f730f4d to db4a59e Compare January 4, 2024 18:44
@wdconinc wdconinc force-pushed the 1166-trackparamtruthinitconfig-assumes-acts-unit-system branch from e45de40 to ce75dfc Compare January 8, 2024 01:50
@wdconinc wdconinc force-pushed the 1166-trackparamtruthinitconfig-assumes-acts-unit-system branch from ce75dfc to cb4d891 Compare January 8, 2024 17:41
@wdconinc
Copy link
Contributor Author

No diffs.

@veprbl
Copy link
Member

veprbl commented Jan 12, 2024

No diffs.

Is that with running capybara locally?
I see diffs on the web pages right now.

@wdconinc
Copy link
Contributor Author

Might be that webpages are out of sync. Last CI run eg https://github.com/eic/EICrecon/actions/runs/7503697056/job/20430403477 has no diffs.

Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Diff LGTM. We should be transitioning to edm4eic::unit, but now is not the time.

@wdconinc wdconinc added this pull request to the merge queue Jan 12, 2024
@wdconinc
Copy link
Contributor Author

Definitely we should move to edm4eic::units, see #925.

Merged via the queue into main with commit 80b9022 Jan 12, 2024
73 checks passed
@wdconinc wdconinc deleted the 1166-trackparamtruthinitconfig-assumes-acts-unit-system branch January 12, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: tracking Relates to tracking reconstruction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TrackParamTruthInitConfig assumes Acts unit system
2 participants