Skip to content

Conversation

@msmk0
Copy link
Contributor

@msmk0 msmk0 commented Jun 25, 2020

Add Vector4{F,D} types and related transformations and extend the CoordinateIndices enum. A general guideline is added to the documentation outlining the preferred way to consistently access vector/matrix components.

This prepares for the removal of the dedicated SpacePoint type and closes #143.

@msmk0 msmk0 added Component - Core Affects the Core module Improvement Changes to an existing feature labels Jun 25, 2020
@msmk0 msmk0 added this to the v0.28.00 milestone Jun 25, 2020
@msmk0 msmk0 force-pushed the add-vector4 branch 2 times, most recently from 0b6cd55 to 337189c Compare June 25, 2020 10:41
Copy link
Contributor

@FabianKlimpel FabianKlimpel left a comment

Choose a reason for hiding this comment

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

Will there be a follow up PR that changes the accessors in the existing code?

@msmk0
Copy link
Contributor Author

msmk0 commented Jun 25, 2020

Will there be a follow up PR that changes the accessors in the existing code?

There will be one at some point, but this is low on my priority list. This might a good first issue for newcomers.

@msmk0 msmk0 marked this pull request as draft June 25, 2020 19:50
@msmk0
Copy link
Contributor Author

msmk0 commented Jun 25, 2020

Switched to draft. This needs a bit more cleanup in the rest of the code base to make it coherent.

@msmk0 msmk0 marked this pull request as ready for review June 26, 2020 08:51
@msmk0
Copy link
Contributor Author

msmk0 commented Jun 26, 2020

@FabianKlimpel I realized that this only makes sense if the updated types and enum are consistently used throughout the code base. I added the obvious changes to the PR. Could you have another look?

@msmk0 msmk0 force-pushed the add-vector4 branch 2 times, most recently from 3727c27 to 91cb71c Compare June 26, 2020 09:32
Copy link
Contributor

@FabianKlimpel FabianKlimpel left a comment

Choose a reason for hiding this comment

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

Some (I guess) missing replacements and a few style corrections. I'm happy overall for applying what the codeguide wants you to do.

@codecov
Copy link

codecov bot commented Jun 26, 2020

Codecov Report

Merging #286 into master will increase coverage by 3.51%.
The diff coverage is 49.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
+ Coverage   44.84%   48.36%   +3.51%     
==========================================
  Files         376      318      -58     
  Lines       18866    16331    -2535     
  Branches     8970     7572    -1398     
==========================================
- Hits         8461     7898     -563     
+ Misses       4905     3178    -1727     
+ Partials     5500     5255     -245     
Impacted Files Coverage Δ
...lude/Acts/EventData/SingleBoundTrackParameters.hpp 74.41% <ø> (ø)
...cts/EventData/SingleCurvilinearTrackParameters.hpp 62.85% <ø> (ø)
...e/include/Acts/EventData/SingleTrackParameters.hpp 75.00% <ø> (+2.14%) ⬆️
...Acts/EventData/detail/initialize_parameter_set.hpp 81.81% <ø> (ø)
Core/include/Acts/Fitter/GainMatrixUpdater.hpp 20.00% <ø> (ø)
...nclude/Acts/Fitter/detail/VoidKalmanComponents.hpp 100.00% <ø> (ø)
Core/include/Acts/Propagator/AtlasStepper.hpp 68.45% <0.00%> (-0.26%) ⬇️
Core/include/Acts/Propagator/DefaultExtension.hpp 39.72% <0.00%> (ø)
...lude/Acts/Propagator/DenseEnvironmentExtension.hpp 59.64% <0.00%> (ø)
Core/include/Acts/Surfaces/AnnulusBounds.hpp 55.26% <ø> (ø)
... and 100 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32adbd1...c6e2dd0. Read the comment docs.

Co-authored-by: Fabian Klimpel <fabian.klimpel@tum.de>
msmk0 and others added 5 commits June 26, 2020 14:34
Co-authored-by: Fabian Klimpel <fabian.klimpel@tum.de>
Co-authored-by: Fabian Klimpel <fabian.klimpel@tum.de>
Co-authored-by: Fabian Klimpel <fabian.klimpel@tum.de>
Co-authored-by: Fabian Klimpel <fabian.klimpel@tum.de>
@msmk0
Copy link
Contributor Author

msmk0 commented Jun 26, 2020

Thanks for catching that. Should be all in now.

@msmk0
Copy link
Contributor Author

msmk0 commented Jun 26, 2020

@FabianKlimpel I solved the merge conflicts. Could you re-approve?

@FabianKlimpel
Copy link
Contributor

@FabianKlimpel I solved the merge conflicts. Could you re-approve?

Here it says that is still approved.

@msmk0
Copy link
Contributor Author

msmk0 commented Jun 26, 2020

Even better. Then I just wait for the CI to finish. Thanks.

@FabianKlimpel
Copy link
Contributor

Even better. Then I just wait for the CI to finish. Thanks.

I think I saw this before but never really tested whether this really counts as approve, so please test it and hope that it doesn't work. Otherwise this would allow dangerous things.

@msmk0 msmk0 merged commit 207182b into acts-project:master Jun 26, 2020
@msmk0 msmk0 deleted the add-vector4 branch July 2, 2020 07:39
@msmk0 msmk0 mentioned this pull request Jul 2, 2020
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jul 13, 2020
Add Vector4{F,D} types and related transformations and extend the `CoordinateIndices` enum. Use the fixed-size vector/matrix types consistently in the code base. Add a general guideline to the documentation outlining the preferred way to consistently access vector/matrix components.
msmk0 added a commit that referenced this pull request Jul 21, 2020
This removes all `SpacePoint` related types and definitions and replaces them with `Vector4D` and related types. Access to the vector components is always performed via enum values and the unused vector helpers are removed.

This is a followup to #143 and #286.
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 Improvement Changes to an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Remove AxisDefs enum

2 participants