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: Reuse JacobianEngine #2789

Merged
merged 16 commits into from
Jan 19, 2024

Conversation

andiwand
Copy link
Contributor

In #2780 I realized that we do not call any of the functions in JacobianEngine on the current main branch and most of them are duplicated in CovarianceEngine. Here I try to unify the duplicated code again

blocked by

@andiwand andiwand added the 🛑 blocked This item is blocked by another item label Dec 11, 2023
@andiwand andiwand added this to the next milestone Dec 11, 2023
@github-actions github-actions bot added Component - Core Affects the Core module Component - Plugins Affects one or more Plugins labels Dec 11, 2023
Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: 63 lines in your changes are missing coverage. Please review.

Comparison is base (6887847) 48.97% compared to head (c9a6ed3) 49.03%.

Files Patch % Lines
Core/src/Propagator/detail/JacobianEngine.cpp 20.37% 6 Missing and 37 partials ⚠️
Core/src/Propagator/detail/CovarianceEngine.cpp 37.03% 0 Missing and 17 partials ⚠️
...nclude/Acts/Propagator/detail/LoopStepperUtils.hpp 60.00% 1 Missing and 1 partial ⚠️
Core/src/Propagator/StraightLineStepper.cpp 75.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2789      +/-   ##
==========================================
+ Coverage   48.97%   49.03%   +0.06%     
==========================================
  Files         492      492              
  Lines       28593    28526      -67     
  Branches    13524    13477      -47     
==========================================
- Hits        14004    13989      -15     
+ Misses       4825     4819       -6     
+ Partials     9764     9718      -46     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot removed the Component - Plugins Affects one or more Plugins label Dec 18, 2023
@andiwand andiwand removed the 🛑 blocked This item is blocked by another item label Jan 15, 2024
@acts-project-service
Copy link
Collaborator

acts-project-service commented Jan 16, 2024

✅ Athena integration test results [9f4a569]

✅ All tests successful

status job report
🟢 run_unit_tests
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsBenchmarkWithSpot.sh 8 100
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsWorkflow.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateAmbiguityResolution.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateResolvedTracks.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateTracks.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateActsCoreSpacePoints.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateActsSpacePoints.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateSeeds.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateOrthogonalSeeds.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateClusters.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsPersistifyEDM.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsGSFRefitting.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsKfRefitting.sh
🟢 run_ci_tests: python3 ../athena/Tracking/Acts/ActsGeometry/test/ActsExtrapolationAlgTest.py
🟢 run_ci_tests: python3 ../athena/Tracking/Acts/ActsGeometry/test/ActsITkTest.py
🟢 run_workflow_tests_run4_mc
🟢 run_workflow_tests_run2_mc
🟢 run_workflow_tests_run2_data
🟢 run_workflow_tests_run3_mc
🟢 run_workflow_tests_run3_data
🟢 run_art_test: test_data18_13TeV_1000evt
🟢 run_art_test: test_ttbarPU40_reco

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Jan 16, 2024
@paulgessinger
Copy link
Member

This breaks Athena compilation. I guess technically, since this is in detail, we would not call it a breaking change, but de-facto it seems to be used publicly. Maybe this indicates we need to move this OUT of detail, and hence mark this breaking?

@andiwand
Copy link
Contributor Author

This breaks Athena compilation. I guess technically, since this is in detail, we would not call it a breaking change, but de-facto it seems to be used publicly. Maybe this indicates we need to move this OUT of detail, and hence mark this breaking?

puh that is unexpected. I was hoping that this is not used outside. do you know where it is used and how extensively?

@andiwand andiwand removed the Breaks Athena build This PR breaks the Athena build label Jan 19, 2024
@kodiakhq kodiakhq bot merged commit 9f4a569 into acts-project:main Jan 19, 2024
53 checks passed
@andiwand andiwand deleted the reuse-jacobian-eninge branch January 19, 2024 13:49
@paulgessinger paulgessinger modified the milestones: next, v32.0.0 Jan 19, 2024
@paulgessinger paulgessinger added the Breaks Athena build This PR breaks the Athena build label Jan 23, 2024
LaraCalic pushed a commit to LaraCalic/acts that referenced this pull request Feb 10, 2024
In acts-project#2780 I realized that we do not call any of the functions in `JacobianEngine` on the current main branch and most of them are duplicated in `CovarianceEngine`. Here I try to unify the duplicated code again

blocked by
- acts-project#2782
- acts-project#2781
kodiakhq bot pushed a commit that referenced this pull request Mar 28, 2024
…d of bound vector (#2811)

This avoid free->bound->free roundtrips and aligns the interface with `freeToBoundJacobian`

related issues
- #2810

blocked by
- #2789
- #2782
- #2781
kodiakhq bot pushed a commit that referenced this pull request Apr 9, 2024
I noticed that the concept of a curvilinear surface is a quite spread over the codebase and I wanted to improve this. Here I introduce a surface like class which does not actually inherit from `Acts::Surface` but shares some of the functionality. This way the jacobians are put in a single place and can be used from somewhere else in an expressive fashion.

Afterwards it might make sense to let the `CurvilinearTrackParameters` depend on this special surface instead of a `PlaneSurface` or to merge them with `BoundTrackParameters` completely.

related issues
- #2812 

blocked by
- #2789
- #2782
- #2781
- #2811
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Apr 19, 2024
I noticed that the concept of a curvilinear surface is a quite spread over the codebase and I wanted to improve this. Here I introduce a surface like class which does not actually inherit from `Acts::Surface` but shares some of the functionality. This way the jacobians are put in a single place and can be used from somewhere else in an expressive fashion.

Afterwards it might make sense to let the `CurvilinearTrackParameters` depend on this special surface instead of a `PlaneSurface` or to merge them with `BoundTrackParameters` completely.

related issues
- acts-project#2812 

blocked by
- acts-project#2789
- acts-project#2782
- acts-project#2781
- acts-project#2811
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…d of bound vector (acts-project#2811)

This avoid free->bound->free roundtrips and aligns the interface with `freeToBoundJacobian`

related issues
- acts-project#2810

blocked by
- acts-project#2789
- acts-project#2782
- acts-project#2781
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
I noticed that the concept of a curvilinear surface is a quite spread over the codebase and I wanted to improve this. Here I introduce a surface like class which does not actually inherit from `Acts::Surface` but shares some of the functionality. This way the jacobians are put in a single place and can be used from somewhere else in an expressive fashion.

Afterwards it might make sense to let the `CurvilinearTrackParameters` depend on this special surface instead of a `PlaneSurface` or to merge them with `BoundTrackParameters` completely.

related issues
- acts-project#2812 

blocked by
- acts-project#2789
- acts-project#2782
- acts-project#2781
- acts-project#2811
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…d of bound vector (acts-project#2811)

This avoid free->bound->free roundtrips and aligns the interface with `freeToBoundJacobian`

related issues
- acts-project#2810

blocked by
- acts-project#2789
- acts-project#2782
- acts-project#2781
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
I noticed that the concept of a curvilinear surface is a quite spread over the codebase and I wanted to improve this. Here I introduce a surface like class which does not actually inherit from `Acts::Surface` but shares some of the functionality. This way the jacobians are put in a single place and can be used from somewhere else in an expressive fashion.

Afterwards it might make sense to let the `CurvilinearTrackParameters` depend on this special surface instead of a `PlaneSurface` or to merge them with `BoundTrackParameters` completely.

related issues
- acts-project#2812 

blocked by
- acts-project#2789
- acts-project#2782
- acts-project#2781
- acts-project#2811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaks Athena build This PR breaks the Athena build Component - Core Affects the Core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants