fix: Properly sort candidates in NavigationStateUpdators
#2483
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
An indexed surfaces multilayer navigation Remove unused variable Remove unused variable fix type conversions for the number of bins changes on the mockupbuilder header file and on the unit test Update Core/include/Acts/Navigation/MultiWireLayerUpdators.hpp Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com> Change on test mockup builder script Multi Wire structure with the interface Changes on the multiwire structure builder Place the files that create the mockup geometry in another folder Change the location of the gdml file An indexed surfaces multilayer navigation Remove unused variable fix type conversions for the number of bins Multi Wire structure with the interface Delete MultiWireLayerUpdators.hpp Update CMakeLists.txt revert some files trying for Indexed Surfaces Generator Indexed Surfaces generator update fix LayerStructure builder fix fix Delete MuonChamber.gdml revert layer strucutre builder from upstream reslove conflict Multi Layer Builder cmake file Place the files that create the mockup geometry in another folder Change the location of the gdml file An indexed surfaces multilayer navigation Remove unused variable Remove unused variable fix type conversions for the number of bins Remove some actsvg includes not needed now change the path for the gdml file changes on the mockupbuilder header file and on the unit test Update Core/include/Acts/Navigation/MultiWireLayerUpdators.hpp Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com> Change on test mockup builder script Multi Wire structure with the interface Changes on the multiwire structure builder Place the files that create the mockup geometry in another folder Change the location of the gdml file An indexed surfaces multilayer navigation Remove unused variable fix type conversions for the number of bins Remove some actsvg includes not needed now change the path for the gdml file Multi Wire structure with the interface Delete MultiWireLayerUpdators.hpp Delete IndexedSurfacesNavigationTests.cpp Update CMakeLists.txt revert some files trying for Indexed Surfaces Generator Indexed Surfaces generator update fix LayerStructure builder fix fix Delete MuonChamber.gdml revert layer strucutre builder from upstream cmake file Update MultiWireStructureBuilder.hpp Update MultiWireStructureBuilder.hpp Update MultiWireStructureBuilder.cpp Place the files that create the mockup geometry in another folder Change the location of the gdml file An indexed surfaces multilayer navigation Remove unused variable Remove unused variable fix type conversions for the number of bins Remove some actsvg includes not needed now change the path for the gdml file changes on the mockupbuilder header file and on the unit test Update Core/include/Acts/Navigation/MultiWireLayerUpdators.hpp Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com> Change on test mockup builder script Multi Wire structure with the interface Changes on the multiwire structure builder Place the files that create the mockup geometry in another folder Change the location of the gdml file An indexed surfaces multilayer navigation Remove unused variable fix type conversions for the number of bins Remove some actsvg includes not needed now change the path for the gdml file Multi Wire structure with the interface Delete MultiWireLayerUpdators.hpp Delete IndexedSurfacesNavigationTests.cpp Update CMakeLists.txt revert some files trying for Indexed Surfaces Generator Indexed Surfaces generator update LayerStructure builder fix fix Delete MuonChamber.gdml revert gdml from upstream revert layer strucutre builder from upstream fix conflicts and some optimizations conflicts and format revert some files revert some files new updator fix license issue fix
dimitra97
force-pushed
the
updators-fix
branch
from
e0303bd
to
3786c83
Compare
Codecov Report
@@ Coverage Diff @@
## main #2483 +/- ##
==========================================
- Coverage 49.85% 49.84% -0.01%
==========================================
Files 466 466
Lines 26253 26250 -3
Branches 12034 12034
==========================================
- Hits 13089 13085 -4
Misses 4615 4615
- Partials 8549 8550 +1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
andiwand
changed the title
NavigationStateUpdators
|
With #2316 I would move the sorting to the navigator so the job of the delegate is just to provide the candidates. But I haven't followed up on this effort in a while so happy to merge your fix in first. |
dimitra97
force-pushed
the
updators-fix
branch
from
95130dc
to
f6634e3
Compare
I think then the sort flag is not needed? |
|
yes exactly I think this would simplify things |
andiwand
previously approved these changes
Sep 27, 2023
Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com>
andiwand
approved these changes
Sep 27, 2023
|
doc build timed out again, but this I will overrule if that's the only check that fails. |
AJPfleger
pushed a commit
to AJPfleger/acts
that referenced
this pull request
Sep 29, 2023
…ect#2483) This PR introduces some changes in the surface candidates updators. I moved some functions from the `SurfaceCandidatesUpdator.hpp` to the `NavigationStateUpdators.hpp` so they can be visible to the updators not defined in the `SurfaceCandidatesUpdators.hpp` The flag `sort` indicates whether sorting is needed or not. A case where the individual updators should not do sorting is when they are passed as updators to the `Chained Updator`, where the sorting needs to be done in the end in the `Chained Updator`. I think doing twice the sorting when using the` Chained Updator` takes a lot of time. --------- Co-authored-by: Dimitra Amperiadou <dimitra@dimitra-tosh.station> Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
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.
This PR introduces some changes in the surface candidates updators.
I moved some functions from the
SurfaceCandidatesUpdator.hppto theNavigationStateUpdators.hppso they can be visible to the updators not defined in theSurfaceCandidatesUpdators.hppThe flag
sortindicates whether sorting is needed or not. A case where the individual updators should not do sorting is when they are passed as updators to theChained Updator, where the sorting needs to be done in the end in theChained Updator.I think doing twice the sorting when using the
Chained Updatortakes a lot of time.