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

Replace auto keywords in KalmanVertexUpdaters by explicit types #235

Conversation

baschlag
Copy link
Contributor

@baschlag baschlag commented Jun 8, 2020

This PR removes all unnecessary auto keywords in the KalmanVertexUpdater and KalmanVertexTrackUpdater and replaces them with the explicit types instead. Some recent, already fixed (and extremely hard to find) bugs appeared only in release (and not debug) build mode due to weird behaviour in the combined usage of auto and Eigens .block<> operations (in a different place).
//Edit: AMVFitter and AMVFinder have also been updated.

@acts-issue-bot acts-issue-bot bot added the Triage label Jun 8, 2020
@baschlag baschlag added Component - Core Affects the Core module Impact - Minor Nuissance bug and/or affects only a single module Improvement Changes to an existing feature and removed Triage labels Jun 8, 2020
@asalzburger
Copy link
Contributor

Hi @baschlag - yes, I think this is the lazy evaluation of Eigen, I tend to use auto for retrieval types, but not very much anymore for Eigen.

Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

Happy to approve this one.

@baschlag
Copy link
Contributor Author

baschlag commented Jun 8, 2020

Happy to approve this one.

Thanks! I found one more case where I'm doing this. I will quickly add it to this PR.

@baschlag baschlag requested a review from asalzburger June 8, 2020 20:21
@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #235 into master will decrease coverage by 0.02%.
The diff coverage is 5.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #235      +/-   ##
==========================================
- Coverage   44.99%   44.96%   -0.03%     
==========================================
  Files         374      374              
  Lines       18697    18709      +12     
  Branches     8842     8854      +12     
==========================================
  Hits         8413     8413              
  Misses       4906     4906              
- Partials     5378     5390      +12     
Impacted Files Coverage Δ
...ore/include/Acts/Vertexing/KalmanVertexUpdater.ipp 28.57% <0.00%> (-4.19%) ⬇️
...nclude/Acts/Vertexing/KalmanVertexTrackUpdater.ipp 17.91% <13.33%> (-0.43%) ⬇️

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 661c107...9d4536e. Read the comment docs.

@paulgessinger
Copy link
Member

Coverage check seems to be stuck. Seems fine though. Merging.

@paulgessinger paulgessinger merged commit 6e4302f into acts-project:master Jun 9, 2020
@paulgessinger paulgessinger added this to the v0.26.00 milestone Jun 11, 2020
@baschlag baschlag deleted the vertex_updator_remove_auto_keywords branch June 23, 2020 12:09
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jul 13, 2020
…-project#235)

* remove auto keywords in KalmanVertex*Updaters and replace with explicit types

* replace auto keywords with explicit types in AMVFinder and AMVFitter
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 - Minor Nuissance bug and/or affects only a single module Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants