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

Consistent track parameters names (Part 1) #241

Merged

Conversation

msmk0
Copy link
Contributor

@msmk0 msmk0 commented Jun 11, 2020

Rename track parameter classes as decided in #151.

This renames the Single...TrackParameters base classes, the neutral typedefs, and the charge free parameters typedef. The {Bound,Curvilinear}Parameters are left for a second PR to reduce this PRs size.

@msmk0 msmk0 added Component - Core Affects the Core module Impact - Major Significant bug and/or affects a lot of modules Improvement Changes to an existing feature labels Jun 11, 2020
@acts-issue-bot acts-issue-bot bot removed the Triage label Jun 11, 2020
@msmk0 msmk0 marked this pull request as draft June 11, 2020 13:23
@msmk0 msmk0 force-pushed the 151-consistent-track-parameters-names-part1 branch 4 times, most recently from 78b60b8 to 9093c72 Compare June 15, 2020 09:01
@msmk0
Copy link
Contributor Author

msmk0 commented Jun 15, 2020

The coverage debug build is failling for unknown reasons and there are no logs or error messages. Does anyone have an idea? Could this be the disk space issue again?

@msmk0 msmk0 mentioned this pull request Jun 15, 2020
@msmk0 msmk0 force-pushed the 151-consistent-track-parameters-names-part1 branch from 9093c72 to 2730f9c Compare June 17, 2020 09:16
@msmk0 msmk0 marked this pull request as ready for review June 17, 2020 09:20
@codecov
Copy link

codecov bot commented Jun 17, 2020

Codecov Report

Merging #241 into master will decrease coverage by 0.04%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #241      +/-   ##
==========================================
- Coverage   48.41%   48.37%   -0.05%     
==========================================
  Files         319      320       +1     
  Lines       16336    16381      +45     
  Branches     7570     7587      +17     
==========================================
+ Hits         7909     7924      +15     
- Misses       3175     3189      +14     
- Partials     5252     5268      +16     
Impacted Files Coverage Δ
...e/include/Acts/EventData/SingleTrackParameters.hpp 75.00% <ø> (+2.14%) ⬆️
Core/src/Material/SurfaceMaterialMapper.cpp 10.20% <0.00%> (ø)
Core/src/Material/VolumeMaterialMapper.cpp 11.69% <0.00%> (ø)
Core/include/Acts/EventData/Measurement.hpp 63.82% <100.00%> (ø)
...clude/Acts/EventData/SingleFreeTrackParameters.hpp 42.55% <100.00%> (ø)
Core/include/Acts/EventData/TrackState.hpp 55.55% <0.00%> (-2.78%) ⬇️
...include/Acts/TrackFinder/CKFSourceLinkSelector.hpp 46.37% <0.00%> (-1.45%) ⬇️
...lude/Acts/Visualization/EventDataVisualization.hpp 40.00% <0.00%> (-1.00%) ⬇️
Core/include/Acts/Propagator/AtlasStepper.hpp 68.45% <0.00%> (-0.26%) ⬇️
...cts/EventData/SingleCurvilinearTrackParameters.hpp 62.85% <0.00%> (ø)
... and 2 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 d8ed32c...a5b74f6. Read the comment docs.

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.

Pretty much strait forward so I don't have much comment on this one.

@msmk0 msmk0 force-pushed the 151-consistent-track-parameters-names-part1 branch from 2730f9c to 46a8bc7 Compare June 18, 2020 08:09
@msmk0
Copy link
Contributor Author

msmk0 commented Jun 18, 2020

@Corentin-Allaire I fixed the typo and hopefully answered the outstanding questions.

@Corentin-Allaire
Copy link
Contributor

That is all good for me. Do we want to let Fabian have a look at it ?

@FabianKlimpel
Copy link
Contributor

That is all good for me. Do we want to let Fabian have a look at it ?

I'm reading it atm

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.

I have one question and some line change suggestions. Beside of that I'm fine

@msmk0 msmk0 force-pushed the 151-consistent-track-parameters-names-part1 branch from c6079e1 to a764e07 Compare June 18, 2020 11:30
@msmk0 msmk0 force-pushed the 151-consistent-track-parameters-names-part1 branch from a764e07 to a5b74f6 Compare June 19, 2020 07:26
@msmk0 msmk0 merged commit ca552e4 into acts-project:master Jun 19, 2020
@paulgessinger paulgessinger added this to the v0.27.00 milestone Jun 25, 2020
@msmk0 msmk0 deleted the 151-consistent-track-parameters-names-part1 branch June 26, 2020 10:57
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jul 13, 2020
* EventData: rename free track parameters
* EventData: remove unqualified track parameters typedef
* EventData: rename neutral parameters header
* EventData: rename charge parameters implementation file
* EventData: remove unneeded cstors/dstore from free params
* EventData: fix explicit template instantiations
* EventData: rename neutral typedefs
* EventData: remove unnecessary include
* EventData: rename charged free params typedef
* Examples: fix track parameters type
* Tests: rename track param tests
* Tests: fix track parameters type
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 Impact - Major Significant bug and/or affects a lot of modules Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants