-
Notifications
You must be signed in to change notification settings - Fork 176
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!: Unify floating point type for chi2 in Track(State) #2562
refactor!: Unify floating point type for chi2 in Track(State) #2562
Conversation
This was previously (not really intentionally) different.
I'm changing the track state type to float first. I'm guessing this has an impact on the fit results, but let's see. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2562 +/- ##
=======================================
Coverage 49.73% 49.73%
=======================================
Files 474 474
Lines 26872 26872
Branches 12364 12364
=======================================
Hits 13366 13366
Misses 4702 4702
Partials 8804 8804 ☔ View full report in Codecov by Sentry. |
Seems like output hashes change (makes sense), but physmon is unchanged. |
This PR currently has a merge conflict. Please resolve this and then re-add the |
Is this breaking actually? Because it touches the concept and so on? |
@benjaminhuth Depends I guess. |
Failures after #2094 went in, as this still expects the other type. |
Just in case this would need to be changed once more is there some Acts::Scalar type for float that could be used instead of explicity type? |
@tboldagh There is, but I'm wondering if that makes sense. Is the goal really to have everything use the same precision? I can't imagine we would actually ever need |
I was acually suggesting (indirectly and eveidently not very suggestive way) that a type: Acts::SmallScalar could be defined and used in this case. |
@tboldagh Seems like a wider discussion. Should we merge this to start with so we can land it in Athena soon? |
Yes. Definitely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this looks alright.
Maybe I tag @CarloVarni - we will need to update xAOD class when this arrives in ATLAS.
This is breaking change (in a not so pleasent way - missing update will result in quite obscure runtime issue). If I understood well the discussion at last ACTS developers, there was plan to flag such breaking changes.
Just to be clear, the effect of this PR is that the TrackState chi2 type changes from double to float, right? The v31.0.0 release notes has it the other way round in the Breaking section. Can the release notes be fixed, or it made clear at the top of this PR? |
@timadye My mistake, sorry! Fixed in the release notes now. |
This was previously (not really intentionally) different.