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: explicit init, pre step, post step for navigator interface #1984

Merged

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Mar 23, 2023

my attempt to make the navigator code more readable. list of changes:

  • explicit init instead of check and call in status
  • status -> postStep because that's what is actually does and why it is called
  • target -> preStep same as above

explicit init
status -> post step
target -> pre step
@andiwand andiwand added the Component - Core Affects the Core module label Mar 23, 2023
@andiwand andiwand added this to the next milestone Mar 23, 2023
@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Merging #1984 (32f9f2c) into main (765cad0) will decrease coverage by 0.03%.
The diff coverage is 54.01%.

❗ Current head 32f9f2c differs from pull request most recent head 0779586. Consider uploading reports for the commit 0779586 to get more accurate results

@@            Coverage Diff             @@
##             main    #1984      +/-   ##
==========================================
- Coverage   49.41%   49.39%   -0.03%     
==========================================
  Files         441      441              
  Lines       25176    25175       -1     
  Branches    11617    11616       -1     
==========================================
- Hits        12441    12434       -7     
- Misses       4481     4485       +4     
- Partials     8254     8256       +2     
Impacted Files Coverage Δ
Core/include/Acts/Propagator/Propagator.ipp 40.25% <33.33%> (-0.77%) ⬇️
Core/include/Acts/Navigation/NextNavigator.hpp 47.65% <38.63%> (-2.35%) ⬇️
Core/include/Acts/Propagator/DirectNavigator.hpp 75.00% <54.54%> (+0.75%) ⬆️
Core/include/Acts/Propagator/Navigator.hpp 58.07% <61.29%> (-1.36%) ⬇️
...cts/Propagator/detail/VoidPropagatorComponents.hpp 91.66% <100.00%> (+0.75%) ⬆️

... and 1 file with indirect coverage changes

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

@github-actions
Copy link

github-actions bot commented Mar 23, 2023

📊 Physics performance monitoring for 0779586

Summary
Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
AMVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

AMVF seeded

AMVF truth_smeared

AMVF truth_estimated

AMVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@andiwand
Copy link
Contributor Author

do you have some feedback on this @paulgessinger @benjaminhuth @asalzburger ?

@andiwand andiwand added the 🚧 WIP Work-in-progress label Mar 24, 2023
@stale
Copy link

stale bot commented Apr 26, 2023

This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.

@stale stale bot added the Stale label Apr 26, 2023
@stale stale bot removed the Stale label Apr 26, 2023
@andiwand
Copy link
Contributor Author

could one of you have a look at this @asalzburger @paulgessinger - would also okay for me to close if you do not think it is relevant enough

@paulgessinger
Copy link
Member

Could still be good for clarity, but I see the added complexity. @asalzburger what do you think?

@benjaminhuth
Copy link
Member

I would actually like this to go in, because everytime I need to do something in the navigator, I need to figure out myself again, what the status and target calls actually do...

@andiwand
Copy link
Contributor Author

This one actually only renames the functions and reorders the code in the navigator. That's why there is a lot of diff

The other PR #1987 was a bit more involved but I closed it now

@andiwand andiwand removed the 🚧 WIP Work-in-progress label Jun 13, 2023
Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Very nice. Only a minor code readablility suggestion, feel free to ignore it!

Core/include/Acts/Propagator/Navigator.hpp Show resolved Hide resolved
@kodiakhq kodiakhq bot merged commit b7b8168 into acts-project:main Jun 13, 2023
51 checks passed
@paulgessinger paulgessinger modified the milestones: next, v27.0.0 Jun 20, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants