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: Remove unit C #2142

Merged
merged 6 commits into from
Jul 8, 2023
Merged

fix: Remove unit C #2142

merged 6 commits into from
Jul 8, 2023

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented May 23, 2023

Not sure what this is and why it was there in this place?

Note
This contains a very odd fix for a problem I was facing in the integration test which compares the analytical covariance with the numerical one. For some reason the numerical derivative breaks down in the case where we step towards a surface which is 1 cm away with a single step. Using 0.9 cm it works because it will do a second step which seems to provide the necessary accuracy for the differential quotient.
This is not pretty and I would like to take a second look why a single step is not sufficient in this case.

@andiwand andiwand added Component - Core Affects the Core module Impact - Critical Critical bug that blocks further developments 🚧 WIP Work-in-progress labels May 23, 2023
@andiwand andiwand added this to the next milestone May 23, 2023
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #2142 (2b5899d) into main (5b1de63) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2142      +/-   ##
==========================================
- Coverage   49.28%   49.27%   -0.01%     
==========================================
  Files         450      450              
  Lines       25410    25408       -2     
  Branches    11727    11727              
==========================================
- Hits        12523    12521       -2     
  Misses       4549     4549              
  Partials     8338     8338              
Impacted Files Coverage Δ
Core/include/Acts/Definitions/Units.hpp 100.00% <ø> (ø)
...agator/detail/GenericDenseEnvironmentExtension.hpp 52.70% <100.00%> (-0.32%) ⬇️

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

@paulgessinger
Copy link
Member

This probably needs a bit of discussion to understand it fully.

@andiwand
Copy link
Contributor Author

there are quite some significant differences now in the integration test... not sure what was tested here before but it looks like kQoP[i]~0 for i>1

maybe we can discuss this tomorrow @asalzburger @paulgessinger

@AJPfleger
Copy link
Contributor

It should be Coulomb. How to get to the value is explained in PRs #1179 and #1231 . But I don't know if we actually need it as a unit.

I am not sure if we should have hbar and c as units instead of keeping them in the namespace PhysicalConstants, because I think we don't want to set actions and velocities as multiples of hbar or c. (If we would ever use those quantities.)

@github-actions github-actions bot added the Component - Examples Affects the Examples module label Jun 19, 2023
@andiwand
Copy link
Contributor Author

andiwand commented Jun 19, 2023

I don't see any value in having Coulomb and it does not seem to be used in the codebase. It might have been confused for c the speed of light in the code I modified in this PR aswell.

I don't have a strong opinion towards splitting constants into Units and PhysicalConstants or not. I just thought it would be good to set them somewhere for reference. In the python bindings we do not distinguish them so also dropped it here. To me 0.5 * c makes more sense in terms of a unit but as you said we will most likely not use that.

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

📊 Physics performance monitoring for 2b5899d

Summary
Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
AMVF: 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

AMVF seeded

AMVF truth_smeared

AMVF truth_estimated

AMVF 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

@andiwand andiwand removed the 🚧 WIP Work-in-progress label Jul 7, 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 good, I think a change from another PR got in here by accident ?

@kodiakhq kodiakhq bot merged commit d0ad664 into acts-project:main Jul 8, 2023
@github-actions github-actions bot removed the automerge label Jul 8, 2023
@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Jul 8, 2023
@andiwand andiwand deleted the remove-unit-c branch July 8, 2023 08:09
@paulgessinger paulgessinger removed the Breaks Athena build This PR breaks the Athena build label Jul 8, 2023
@paulgessinger
Copy link
Member

This was a false failure due to #2251

kodiakhq bot pushed a commit that referenced this pull request Jul 12, 2023
In #2142 I discovered that the ridders propagator depends highly on the step size taken. At the same time we still use the global static surface tolerance in most of the places which I try to improve here. This allows us to consistently set the surface tolerance for the propagation and to improve the accuracy of the ridders propagator.

Includes a bug fix for `AtlasStepper` I discovered after making these changes. The stepper ignored the stepping tolerance in backward mode.
@paulgessinger paulgessinger modified the milestones: next, v27.2.0 Jul 24, 2023
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jul 24, 2023
Not sure what this is and why it was there in this place?

**Note**
This contains a very odd fix for a problem I was facing in the integration test which compares the analytical covariance with the numerical one. For some reason the numerical derivative breaks down in the case where we step towards a surface which is 1 cm away with a single step. Using 0.9 cm it works because it will do a second step which seems to provide the necessary accuracy for the differential quotient.
This is not pretty and I would like to take a second look why a single step is not sufficient in this case.
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jul 24, 2023
…2292)

In acts-project#2142 I discovered that the ridders propagator depends highly on the step size taken. At the same time we still use the global static surface tolerance in most of the places which I try to improve here. This allows us to consistently set the surface tolerance for the propagation and to improve the accuracy of the ridders propagator.

Includes a bug fix for `AtlasStepper` I discovered after making these changes. The stepper ignored the stepping tolerance in backward mode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Component - Examples Affects the Examples module Impact - Critical Critical bug that blocks further developments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants