feat: Allow setting reverse filter p threshold #1103
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #1103 +/- ##
=======================================
Coverage 48.62% 48.62%
=======================================
Files 341 341
Lines 17511 17511
Branches 8244 8244
=======================================
Hits 8515 8515
Misses 3232 3232
Partials 5764 5764 Continue to review full report at Codecov.
|
asalzburger
approved these changes
Dec 9, 2021
|
There's still a failure & the format checks are running. |
The failure is because #1101 is not in this branch yet. I'll update and it should resolve the failure (as intended). |
|
I also fixed the format now. |
- 0 GeV hashes are from before, these should be still valid - 1 TeV hashes are from local, might be wrong
|
Ok, everything seems to succeed. |
paulgessinger
added
Bug
kodiakhq bot
pushed a commit
that referenced
this pull request
Dec 14, 2021
I noticed in #1103 that the navigation failed in backward mode after we merged #1107. Indeed, during the change in #1107, `TrackingVolume` lost a line that copied the sign from the navigation direction onto the path length in `compatibleBoundaries`. This made the navigation go haywire when it tried to target boundaries in reverse propagation mode. This PR adds that back in, and should fix #1103's test runs.
|
Tests succeeds now it seems. @asalzburger can you approve one more time? |
paulgessinger
added a commit
to paulgessinger/acts
that referenced
this pull request
Dec 15, 2021
…#1115) I noticed in acts-project#1103 that the navigation failed in backward mode after we merged acts-project#1107. Indeed, during the change in acts-project#1107, `TrackingVolume` lost a line that copied the sign from the navigation direction onto the path length in `compatibleBoundaries`. This made the navigation go haywire when it tried to target boundaries in reverse propagation mode. This PR adds that back in, and should fix acts-project#1103's test runs.
|
@asalzburger The bot isn't merging because the approval itself is not valid yet. |
|
Merged by hand. |
paulgessinger
added a commit
to paulgessinger/acts
that referenced
this pull request
Aug 3, 2022
…#1115) I noticed in acts-project#1103 that the navigation failed in backward mode after we merged acts-project#1107. Indeed, during the change in acts-project#1107, `TrackingVolume` lost a line that copied the sign from the navigation direction onto the path length in `compatibleBoundaries`. This made the navigation go haywire when it tried to target boundaries in reverse propagation mode. This PR adds that back in, and should fix acts-project#1103's test runs.
acts-project-service
pushed a commit
that referenced
this pull request
Aug 5, 2022
I noticed in #1103 that the navigation failed in backward mode after we merged #1107. Indeed, during the change in #1107, `TrackingVolume` lost a line that copied the sign from the navigation direction onto the path length in `compatibleBoundaries`. This made the navigation go haywire when it tried to target boundaries in reverse propagation mode. This PR adds that back in, and should fix #1103's test runs. (cherry picked from commit 67afb40)
paulgessinger
added a commit
that referenced
this pull request
Aug 9, 2022
… to develop/v9.1.x] (#1383) fix: TrackingVolume intersection check didn't copy sign (#1115) I noticed in #1103 that the navigation failed in backward mode after we merged #1107. Indeed, during the change in #1107, `TrackingVolume` lost a line that copied the sign from the navigation direction onto the path length in `compatibleBoundaries`. This made the navigation go haywire when it tried to target boundaries in reverse propagation mode. This PR adds that back in, and should fix #1103's test runs. (cherry picked from commit 67afb40) Co-authored-by: Paul Gessinger <paul.gessinger@cern.ch>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We previously didn't set up the reverse filtering at all in the examples track fitting algorithm, so the WARNING in #1101 and #1077 was never observed.
This PR exposes a momentum threshold variable through the config struct and into python. The pytests run it with a threshold of 0 (no reverse filtering at all) and 1 TeV (almost always reverse filtering), so we should at least see this warning here until #1101 is merged.