refactor!: move useBeamSpotConstraint to vertexingOptions #2272

felix-russo · 2023-07-05T09:49:19Z

Previously, the boolean member variable useBeamSpotConstraint of the AMVF and IVF was set independently of the actual beam spot constraint which was part of the vertexingOptions. This PR moves useBeamSpotConstraint to vertexingOptions as well.

codecov · 2023-07-05T13:02:19Z

Codecov Report

Merging #2272 (ec3a1bf) into main (c1e0cb5) will increase coverage by 0.07%.
Report is 2 commits behind head on main.
The diff coverage is 31.57%.

@@            Coverage Diff             @@
##             main    #2272      +/-   ##
==========================================
+ Coverage   49.53%   49.60%   +0.07%     
==========================================
  Files         453      453              
  Lines       25692    25691       -1     
  Branches    11819    11814       -5     
==========================================
+ Hits        12726    12744      +18     
+ Misses       4583     4569      -14     
+ Partials     8383     8378       -5

Files Changed	Coverage Δ
...Acts/Vertexing/AdaptiveGridDensityVertexFinder.ipp	`51.94% <0.00%> (+1.31%)`	⬆️
...clude/Acts/Vertexing/AdaptiveMultiVertexFinder.hpp	`89.47% <ø> (ø)`
...include/Acts/Vertexing/GridDensityVertexFinder.ipp	`50.64% <0.00%> (+1.28%)`	⬆️
...e/include/Acts/Vertexing/IterativeVertexFinder.hpp	`80.00% <ø> (ø)`
...nclude/Acts/Vertexing/TrackDensityVertexFinder.ipp	`35.71% <0.00%> (+2.38%)`	⬆️
...e/include/Acts/Vertexing/IterativeVertexFinder.ipp	`26.69% <8.33%> (-0.51%)`	⬇️
...clude/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp	`41.29% <12.50%> (+0.48%)`	⬆️
Core/include/Acts/Vertexing/ZScanVertexFinder.ipp	`46.15% <14.28%> (+22.51%)`	⬆️
...include/Acts/Vertexing/FullBilloirVertexFitter.ipp	`28.05% <55.55%> (+0.08%)`	⬆️
Core/include/Acts/Vertexing/VertexingOptions.hpp	`76.92% <76.92%> (-23.08%)`	⬇️

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

github-actions · 2023-07-05T14:23:50Z

📊: Physics performance monitoring for `3142a7f`

Full contents

physmon summary

paulgessinger · 2023-07-06T06:15:59Z

I guess this is breaking backward compatibility? Outputs seem unchanged on our end.

felix-russo · 2023-07-06T07:14:16Z

I guess this is breaking backward compatibility? Outputs seem unchanged on our end.

Ah yes indeed it does! I didn't have this in mind when coding (I am still not used to working on such big projects). I guess this is a no-go?

paulgessinger · 2023-07-06T08:51:47Z

No not at all, we just have to handle it correctly. I might have to ask you to do some checks on the Athena side to determine the impact (cc @CarloVarni @noemina)

paulgessinger

Here are a few comments.

Can we get this in before the end of the week in time for v28?

Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.hpp

Core/include/Acts/Vertexing/VertexingOptions.hpp

start removing boolean useBeamSpotConstr Co-authored-by: Paul Gessinger <hello@paulgessinger.com>

…constr

paulgessinger · 2023-08-28T15:52:22Z

/run-experiment atlas

paulgessinger · 2023-08-28T20:16:41Z

The bridged physmon job fails due (at least) 3 FPEs that are encountered. From the ckf_tracking log:

17:41:39    Sequencer      INFO      - FLTUND: (3 times) 
 0# Acts::AdaptiveMultiVertexFinder<Acts::AdaptiveMultiVertexFitter<Acts::GenericBoundTrackParameters<Acts::SinglyCharged>, Acts::HelicalTrackLinearizer<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::detail::GenericDefaultExtension<double> >, Acts::detail::VoidAuctioneer>, Acts::detail::VoidNavigator>, Acts::PropagatorOptions<Acts::ActionList<>, Acts::AbortList<> > > >, Acts::TrackDensityVertexFinder<Acts::AdaptiveMultiVertexFitter<Acts::GenericBoundTrackParameters<Acts::SinglyCharged>, Acts::HelicalTrackLinearizer<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::detail::GenericDefaultExtension<double> >, Acts::detail::VoidAuctioneer>, Acts::detail::VoidNavigator>, Acts::PropagatorOptions<Acts::ActionList<>, Acts::AbortList<> > > >, Acts::GaussianTrackDensity<Acts::GenericBoundTrackParameters<Acts::SinglyCharged> > > >::find(std::vector<Acts::GenericBoundTrackParameters<Acts::SinglyCharged> const*, std::allocator<Acts::GenericBoundTrackParameters<Acts::SinglyCharged> const*> > const&, Acts::VertexingOptions<Acts::GenericBoundTrackParameters<Acts::SinglyCharged> > const&, Acts::AdaptiveMultiVertexFinder<Acts::AdaptiveMultiVertexFitter<Acts::GenericBoundTrackParameters<Acts::SinglyCharged>, Acts::HelicalTrackLinearizer<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::detail::GenericDefaultExtension<double> >, Acts::detail::VoidAuctioneer>, Acts::detail::VoidNavigator>, Acts::PropagatorOptions<Acts::ActionList<>, Acts::AbortList<> > > >, Acts::TrackDensityVertexFinder<Acts::AdaptiveMultiVertexFitter<Acts::GenericBoundTrackParameters<Acts::SinglyCharged>, Acts::HelicalTrackLinearizer<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::detail::GenericDefaultExtension<double> >, Acts::detail::VoidAuctioneer>, Acts::detail::VoidNavigator>, Acts::PropagatorOptions<Acts::ActionList<>, Acts::AbortList<> > > >, Acts::GaussianTrackDensity<Acts::GenericBoundTrackParameters<Acts::SinglyCharged> > > >::State&) const [clone .isra.0] at /builds/acts/ci-bridge/src/Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.ipp:120
 1# ActsExamples::ProcessCode ActsExamples::AdaptiveMultiVertexFinderAlgorithm::executeAfterSeederChoice<Acts::TrackDensityVertexFinder<Acts::AdaptiveMultiVertexFitter<Acts::GenericBoundTrackParameters<Acts::SinglyCharged>, Acts::HelicalTrackLinearizer<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::detail::GenericDefaultExtension<double> >, Acts::detail::VoidAuctioneer>, Acts::detail::VoidNavigator>, Acts::PropagatorOptions<Acts::ActionList<>, Acts::AbortList<> > > >, Acts::GaussianTrackDensity<Acts::GenericBoundTrackParameters<Acts::SinglyCharged> > >, Acts::AdaptiveMultiVertexFinder<Acts::AdaptiveMultiVertexFitter<Acts::GenericBoundTrackParameters<Acts::SinglyCharged>, Acts::HelicalTrackLinearizer<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::detail::GenericDefaultExtension<double> >, Acts::detail::VoidAuctioneer>, Acts::detail::VoidNavigator>, Acts::PropagatorOptions<Acts::ActionList<>, Acts::AbortList<> > > >, Acts::TrackDensityVertexFinder<Acts::AdaptiveMultiVertexFitter<Acts::GenericBoundTrackParameters<Acts::SinglyCharged>, Acts::HelicalTrackLinearizer<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::detail::GenericDefaultExtension<double> >, Acts::detail::VoidAuctioneer>, Acts::detail::VoidNavigator>, Acts::PropagatorOptions<Acts::ActionList<>, Acts::AbortList<> > > >, Acts::GaussianTrackDensity<Acts::GenericBoundTrackParameters<Acts::SinglyCharged> > > > >(ActsExamples::AlgorithmContext const&, Acts::TrackDensityVertexFinder<Acts::AdaptiveMultiVertexFitter<Acts::GenericBoundTrackParameters<Acts::SinglyCharged>, Acts::HelicalTrackLinearizer<Acts::Propagator<Acts::EigenStepper<Acts::StepperExtensionList<Acts::detail::GenericDefaultExtension<double> >, Acts::detail::VoidAuctioneer>, Acts::detail::VoidNavigator>, Acts::PropagatorOptions<Acts::ActionList<>, Acts::AbortList<> > > >, Acts::GaussianTrackDensity<Acts::GenericBoundTrackParameters<Acts::SinglyCharged> > > const&) const at /builds/acts/ci-bridge/src/Examples/Algorithms/Vertexing/src/AdaptiveMultiVertexFinderAlgorithm.cpp:161
 2# ActsExamples::AdaptiveMultiVertexFinderAlgorithm::execute(ActsExamples::AlgorithmContext const&) const at /builds/acts/ci-bridge/src/Examples/Algorithms/Vertexing/src/AdaptiveMultiVertexFinderAlgorithm.cpp:65
 3# ActsExamples::Sequencer::run()::{lambda()#1}::operator()() const::{lambda(tbb::detail::d1::blocked_range<unsigned long> const&)#1}::operator()(tbb::detail::d1::blocked_range<unsigned long> const&) const at /builds/acts/ci-bridge/src/Examples/Framework/src/Framework/Sequencer.cpp:505
 4# tbb::detail::d1::start_for<tbb::detail::d1::blocked_range<unsigned long>, ActsExamples::Sequencer::run()::{lambda()#1}::operator()() const::{lambda(tbb::detail::d1::blocked_range<unsigned long> const&)#1}, tbb::detail::d1::auto_partitioner const>::execute(tbb::detail::d1::execution_data&) at /usr/include/oneapi/tbb/parallel_for.h:172
 5# 0x00007FA411E61B3C in /usr/lib/x86_64-linux-gnu/libtbb.so.12
 6# 0x00007FA411E63DD8 in /usr/lib/x86_64-linux-gnu/libtbb.so.12
 7# 0x00007FA413C07B43 in /usr/lib/x86_64-linux-gnu/libc.so.6

Any ideas @felix-russo?

acts-project-service · 2023-08-29T10:34:11Z

🔴 Athena integration test results

🔴 Some tests have failed!

Please investigate the pipeline!

status	job
🟢	run_unit_tests
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsWorkflow.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateTracks.sh
🔴	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateActsCoreSpacePoints.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateActsSpacePoints.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateSeeds.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateOrthogonalSeeds.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateClusters.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsPersistifyEDM.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsKfRefitting.sh
🟢	run_ci_tests: python3 ../athena/Tracking/Acts/ActsGeometry/test/ActsExtrapolationAlgTest.py
🟢	run_ci_tests: python3 ../athena/Tracking/Acts/ActsGeometry/test/ActsITkTest.py
🟢	run_workflow_tests_run4_mc
🟢	run_workflow_tests_run2_mc
🟢	run_workflow_tests_run2_data
🟢	run_workflow_tests_run3_mc
🟢	run_workflow_tests_run3_data

paulgessinger · 2023-08-29T11:24:45Z

Transient error, breaks (broke) the build though.

move useBeamSpotConstraint to vertexingOptions

a1d896c

github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Documentation Affects the documentation Vertexing labels Jul 5, 2023

felix-russo changed the title ~~move useBeamSpotConstraint to vertexingOptions~~ refactor: move useBeamSpotConstraint to vertexingOptions Jul 5, 2023

paulgessinger added this to the next milestone Jul 5, 2023

paulgessinger added the 🚧 WIP Work-in-progress label Jul 5, 2023

paulgessinger marked this pull request as draft July 5, 2023 11:53

felix-russo added 2 commits July 5, 2023 14:02

remove bug

e798ba8

remove bug

fa91e1e

update FPE mask location

4dcb607

github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Jul 5, 2023

felix-russo marked this pull request as ready for review July 5, 2023 21:00

paulgessinger removed the 🚧 WIP Work-in-progress label Jul 6, 2023

paulgessinger changed the title ~~refactor: move useBeamSpotConstraint to vertexingOptions~~ refactor!: move useBeamSpotConstraint to vertexingOptions Jul 6, 2023

paulgessinger added the Breaking change This change breaks backwards compatibility label Jul 6, 2023

paulgessinger modified the milestones: next, v28.0.0 Jul 6, 2023

paulgessinger reviewed Jul 24, 2023

View reviewed changes

Core/include/Acts/Vertexing/AdaptiveMultiVertexFinder.hpp Outdated Show resolved Hide resolved

Core/include/Acts/Vertexing/VertexingOptions.hpp Outdated Show resolved Hide resolved

Core/include/Acts/Vertexing/VertexingOptions.hpp Outdated Show resolved Hide resolved

paulgessinger modified the milestones: v28.0.0, v29.0.0 Jul 27, 2023

felix-russo and others added 3 commits August 1, 2023 10:53

do not use const refrence for booleans

4302897

always check if covariance is invertible

87268af

Update Core/include/Acts/Vertexing/VertexingOptions.hpp

41cc31f

start removing boolean useBeamSpotConstr Co-authored-by: Paul Gessinger <hello@paulgessinger.com>

felix-russo added 10 commits August 7, 2023 09:02

Merge branch 'main' of https://github.com/acts-project/acts into vtx-…

d413961

…constr

Merge branch 'main' of https://github.com/acts-project/acts into vtx-…

5686de2

…constr

Merge branch 'main' of https://github.com/acts-project/acts into vtx-…

246de3b

…constr

merge branch main in

890ccef

remove bug

8b483dd

Merge branch 'main' of https://github.com/acts-project/acts into vtx-…

c5baccd

…constr

merge branch main into vtx-constraint

ee46b03

Merge branch 'main' of https://github.com/acts-project/acts into vtx-…

9c4fabf

…constr

rename

842e96c

improve comments

ba2a300

paulgessinger previously approved these changes Aug 28, 2023

View reviewed changes

paulgessinger added the automerge label Aug 28, 2023

Merge branch 'main' into vtx-constr

fe167c8

paulgessinger removed the automerge label Aug 28, 2023

change mask of AnnealingUtility

29ed351

paulgessinger dismissed their stale review via 29ed351 August 28, 2023 20:19

paulgessinger added 2 commits August 28, 2023 22:21

another fpe fix (this is probably what caused it)

254789c

add another FLTUND in AnnealingUtility.cpp

ec3a1bf

CarloVarni approved these changes Aug 29, 2023

View reviewed changes

Merge branch 'main' into vtx-constr

3142a7f

paulgessinger merged commit 99050a8 into acts-project:main Aug 29, 2023

acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Aug 29, 2023

paulgessinger added Breaks Athena build This PR breaks the Athena build and removed Fails Athena tests This PR causes a failure in the Athena tests labels Aug 29, 2023

felix-russo deleted the vtx-constr branch October 3, 2023 11:39

felix-russo mentioned this pull request Oct 11, 2023

refactor: make usage of vertex constraint more evident #2529

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor!: move useBeamSpotConstraint to vertexingOptions #2272

refactor!: move useBeamSpotConstraint to vertexingOptions #2272

felix-russo commented Jul 5, 2023 •

edited

Loading

codecov bot commented Jul 5, 2023 •

edited

Loading

github-actions bot commented Jul 5, 2023 •

edited

Loading

paulgessinger commented Jul 6, 2023 •

edited

Loading

felix-russo commented Jul 6, 2023

paulgessinger commented Jul 6, 2023

paulgessinger left a comment

paulgessinger commented Aug 28, 2023

paulgessinger commented Aug 28, 2023

acts-project-service commented Aug 29, 2023

paulgessinger commented Aug 29, 2023

refactor!: move useBeamSpotConstraint to vertexingOptions #2272

refactor!: move useBeamSpotConstraint to vertexingOptions #2272

Conversation

felix-russo commented Jul 5, 2023 • edited Loading

codecov bot commented Jul 5, 2023 • edited Loading

Codecov Report

github-actions bot commented Jul 5, 2023 • edited Loading

📊: Physics performance monitoring for 3142a7f

physmon summary

paulgessinger commented Jul 6, 2023 • edited Loading

felix-russo commented Jul 6, 2023

paulgessinger commented Jul 6, 2023

paulgessinger left a comment

Choose a reason for hiding this comment

paulgessinger commented Aug 28, 2023

paulgessinger commented Aug 28, 2023

acts-project-service commented Aug 29, 2023

🔴 Athena integration test results

🔴 Some tests have failed!

paulgessinger commented Aug 29, 2023

felix-russo commented Jul 5, 2023 •

edited

Loading

codecov bot commented Jul 5, 2023 •

edited

Loading

github-actions bot commented Jul 5, 2023 •

edited

Loading

📊: Physics performance monitoring for `3142a7f`

paulgessinger commented Jul 6, 2023 •

edited

Loading