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

refactor!: MTJ stores measurement as jagged vector #1512

Merged
merged 17 commits into from
Oct 31, 2022
Merged

refactor!: MTJ stores measurement as jagged vector #1512

merged 17 commits into from
Oct 31, 2022

Conversation

paulgessinger
Copy link
Member

@paulgessinger paulgessinger commented Sep 12, 2022 •
edited

Addresses #1516.

 | x x x | x | x x |  ...
   ^       ^     ^
 M1,D=3 M2,D=1 M3,D=2

BREAKING CHANGE: Acts::MultiTrajectory measurement access methods change:

- constexpr auto measurement(IndexType measIdx) const;
+ template <size_t measdim> 
+ constexpr auto measurement(IndexType measIdx) const;

and

- constexpr auto measurementCovariance(IndexType covIdx)
+ template <size_t measdim>
+ constexpr auto measurementCovariance(IndexType covIdx)

@paulgessinger paulgessinger added the 🚧 WIP Work-in-progress label Sep 12, 2022
@paulgessinger paulgessinger added this to the next milestone Sep 12, 2022
@paulgessinger
Copy link
Member Author

This currently includes changes from #1507.

@paulgessinger
Copy link
Member Author

paulgessinger commented Sep 13, 2022 •
edited

CI failures (so far) seem to be a GCC8 bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86594

I think this is ready for review.

@paulgessinger paulgessinger removed the 🚧 WIP Work-in-progress label Sep 13, 2022
@codecov
Copy link

codecov bot commented Sep 13, 2022 •
edited

Codecov Report

Merging #1512 (a66f529) into main (5c07814) will decrease coverage by 0.02%.
The diff coverage is 46.00%.

@@            Coverage Diff             @@
##             main    #1512      +/-   ##
==========================================
- Coverage   48.65%   48.62%   -0.03%     
==========================================
  Files         381      381              
  Lines       20780    20835      +55     
  Branches     9518     9552      +34     
==========================================
+ Hits        10110    10132      +22     
- Misses       4099     4101       +2     
- Partials     6571     6602      +31
Impacted Files Coverage Δ
...de/Acts/TrackFinding/CombinatorialKalmanFilter.hpp 32.51% <0.00%> (-0.09%) ⬇️
.../include/Acts/TrackFinding/MeasurementSelector.hpp 38.09% <ø> (ø)
...re/include/Acts/TrackFitting/GainMatrixUpdater.hpp 10.00% <ø> (ø)
Core/include/Acts/TrackFitting/detail/GsfUtils.hpp 53.33% <ø> (ø)
...ore/include/Acts/Visualization/EventDataView3D.hpp 46.37% <0.00%> (ø)
Core/src/TrackFitting/GainMatrixUpdater.cpp 15.15% <6.89%> (-4.85%) ⬇️
Core/src/TrackFinding/MeasurementSelector.cpp 21.42% <15.38%> (-11.91%) ⬇️
Core/src/TrackFitting/GsfUtils.cpp 27.27% <20.00%> (-17.18%) ⬇️
Core/include/Acts/EventData/MultiTrajectory.hpp 69.41% <61.36%> (-4.30%) ⬇️
...e/include/Acts/EventData/VectorMultiTrajectory.hpp 58.51% <63.33%> (+0.86%) ⬆️
... and 5 more

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

@paulgessinger paulgessinger mentioned this pull request Sep 14, 2022
Open
2 tasks
@paulgessinger
Copy link
Member Author

I pushed a workaround.

@paulgessinger
Copy link
Member Author

@tboldagh thoughts?

@paulgessinger paulgessinger changed the title refactor: MTJ stores measurement as jagged vector refactor!: MTJ stores measurement as jagged vector Sep 22, 2022
@paulgessinger
Copy link
Member Author

Ok I've marked this as backward breaking now.
Should we get this in @tboldagh?

@paulgessinger
Copy link
Member Author

Still need to update the memory stats calculation

@paulgessinger
Copy link
Member Author

Ok I think this is ready now.

@tboldagh would you be able to have another look?

@tboldagh
Copy link
Contributor

Ok I think this is ready now.

@tboldagh would you be able to have another look?

Yes. Today evening.

@github-actions
Copy link

github-actions bot commented Oct 24, 2022 •
edited

📊 Physics performance monitoring for a66f529

Full report
CKF: seeded, truth smeared, truth estimated
IVF: seeded, truth smeared, truth estimated
Ambiguity resolution
Truth tracking

Vertexing

IVF seeded

IVF truth smeared

IVF truth estimated

CKF

seeded

truth smeared

truth estimated

Ambiguity resolution

seeded

Truth tracking

Truth tracking

@paulgessinger paulgessinger mentioned this pull request Oct 24, 2022
Merged
tboldagh
tboldagh approved these changes Oct 24, 2022
View reviewed changes
Copy link
Contributor

@tboldagh tboldagh left a comment

Choose a reason for hiding this comment

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

I have no substantive comments. Just few small requests to make the diagnostics more useful. It at all possible.

Core/include/Acts/EventData/MultiTrajectory.hpp Show resolved Hide resolved
Core/include/Acts/TrackFitting/GainMatrixUpdater.hpp Show resolved Hide resolved
Core/include/Acts/Utilities/Helpers.hpp Show resolved Hide resolved
Core/src/TrackFitting/GainMatrixUpdater.cpp Show resolved Hide resolved
@tboldagh tboldagh self-requested a review October 25, 2022 15:56
@chdsbd chdsbd mentioned this pull request Oct 27, 2022
Open
tboldagh
tboldagh approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@tboldagh tboldagh left a comment

Choose a reason for hiding this comment

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

No comment. I made a mistake in using the gitlab UI.

@paulgessinger
Copy link
Member Author

Seems like kodiak got unstuck? (@benjaminhuth)

@kodiakhq kodiakhq bot merged commit 5b802fc into acts-project:main Oct 31, 2022
@AJPfleger AJPfleger mentioned this pull request Nov 1, 2022
Merged
@paulgessinger paulgessinger modified the milestones: next, v21.0.0 Nov 3, 2022
kodiakhq bot pushed a commit that referenced this pull request Dec 12, 2022
@paulgessinger
refactor!: Switch SourceLink to container (#1616)
5cab50d 
This PR changes the `SourceLink` back from a base class to a concrete type without inheritance. It now contains a type erased upstream sourcelink (right now implement by `std::any`, but we can change this to use something with small buffer optimization).

Contains #1512 at the moment.
pbutti pushed a commit to pbutti/acts that referenced this pull request Dec 13, 2022
@paulgessinger @pbutti
refactor!: Switch SourceLink to container (acts-project#1616)
b73c024 
This PR changes the `SourceLink` back from a base class to a concrete type without inheritance. It now contains a type erased upstream sourcelink (right now implement by `std::any`, but we can change this to use something with small buffer optimization).

Contains acts-project#1512 at the moment.
CarloVarni pushed a commit to CarloVarni/acts that referenced this pull request Dec 22, 2022
@paulgessinger
refactor!: Switch SourceLink to container (acts-project#1616)
b966aa6 
This PR changes the `SourceLink` back from a base class to a concrete type without inheritance. It now contains a type erased upstream sourcelink (right now implement by `std::any`, but we can change this to use something with small buffer optimization).

Contains acts-project#1512 at the moment.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjaminhuth benjaminhuth benjaminhuth left review comments

@tboldagh tboldagh tboldagh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v21.0.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@paulgessinger @tboldagh @benjaminhuth