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.

Sign up for GitHub

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

Jump to bottom

refactor: Unify proxy iterator types #3664

Merged
merged 2 commits into from
Oct 3, 2024
Merged

refactor: Unify proxy iterator types #3664

merged 2 commits into from
Oct 3, 2024

Conversation

stephenswat
Copy link
Member

@stephenswat stephenswat commented Sep 30, 2024 •
edited
Loading

This PR changed scope, and now unifies all proxy iterators under a single template type.

Old PR description

At this point in time, the iterator for MeasurementContainer does not match the std::forward_iterator concept which SonarCloud really wants me to use. In this commit, I make sure that the iterator satisfies the concept.

Unfortunately, this requires a default-initializer, because std::forward_iterator requires std::incrementable which requires std::regular, which requires std::semiregular which in turn requires std::default_initializable which I think is silly.

@github-actions github-actions bot added Component - Examples Affects the Examples module Event Data Model labels Sep 30, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 30, 2024 •
edited
Loading

📊: Physics performance monitoring for ee60730

Full contents

physmon summary

@stephenswat stephenswat force-pushed the feat/measurement_it_forward branch 2 times, most recently from d00ec45 to 20cf5fc Compare September 30, 2024 11:56
andiwand
andiwand previously approved these changes Sep 30, 2024
View reviewed changes
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

Do you think it would be easier / beneficial to define it as a random access iterator?

@stephenswat
Copy link
Member Author

Hmm, probably; I can do that!

@AJPfleger AJPfleger added this to the v37.0.0 milestone Sep 30, 2024
@stephenswat stephenswat force-pushed the feat/measurement_it_forward branch from 20cf5fc to eff6a58 Compare September 30, 2024 14:18
@github-actions github-actions bot added Component - Core Affects the Core module Seeding labels Sep 30, 2024
@stephenswat stephenswat changed the title feat: Make measurement iterator a forward iterator feat: Unify proxy iterator types Sep 30, 2024
@stephenswat
Copy link
Member Author

This PR has changed scope; rather than have it implement random access only for the measurement iterator, it now captures the proxy iterator for all such containers we have; for measurements but also tracks and spacepoints.

@stephenswat stephenswat changed the title feat: Unify proxy iterator types refactor: Unify proxy iterator types Sep 30, 2024
@stephenswat stephenswat force-pushed the feat/measurement_it_forward branch from eff6a58 to a43c26e Compare September 30, 2024 14:20
paulgessinger
paulgessinger reviewed Oct 2, 2024
View reviewed changes
Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

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

Good idea

Core/include/Acts/EventData/TrackContainer.hpp Show resolved Hide resolved
Core/include/Acts/Utilities/Iterator.hpp Outdated Show resolved Hide resolved
@stephenswat stephenswat force-pushed the feat/measurement_it_forward branch 3 times, most recently from 53b62c8 to a30e446 Compare October 2, 2024 15:08
CarloVarni
CarloVarni reviewed Oct 2, 2024
View reviewed changes
Copy link
Collaborator

@CarloVarni CarloVarni left a comment

Choose a reason for hiding this comment

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

The copyright statement has been changed

Core/include/Acts/Utilities/Iterator.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Utilities/TypeTraits.hpp Outdated Show resolved Hide resolved
@stephenswat stephenswat force-pushed the feat/measurement_it_forward branch 2 times, most recently from fd31c85 to b5cd817 Compare October 2, 2024 20:10
@stephenswat
refactor: Unify proxy iterator types
b5cd817 
We have quite a few container types whose iterators are container-index
pairs. These currently have different implementations which can be
fairly easily unified under a single template.
@sonarqubecloud SonarQubeCloud
Copy link

sonarqubecloud bot commented Oct 3, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
64.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@kodiakhq kodiakhq bot merged commit 94e37c0 into acts-project:main Oct 3, 2024
42 checks passed
@github-actions github-actions bot removed the automerge label Oct 3, 2024
@acts-project-service
Copy link
Collaborator

acts-project-service commented Oct 3, 2024 •
edited
Loading

🔴 Athena integration test results [94e37c0]

🔴 Some tests have failed!

Please investigate the pipeline!

status job report
🟢 test_ActsWorkflow
🟢 report_pull_request
🟢 run_art_test: test_data18_13TeV_1000evt
🟢 run_art_test: test_ttbarPU40_reco
🔴 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
🟢 test_ActsDumpGeometryIdentifiers
🟢 test_ActsCheckObjectCountsCachedWorkflow
🟢 test_ActsCheckObjectCountsWorkflow
🟢 test_ActsEFTrackFit
🟢 test_ActsPersistifySeeds
🟢 test_ActsBenchmarkWithSpot
🟢 test_ActsAnalogueClustering
🟢 test_ActsWorkflowHeavyIons
🟢 test_ActsWorkflowFastTracking
🟢 test_ActsWorkflowCached
🟢 test_ActsValidateAmbiguityResolution
🟢 test_ActsValidateResolvedTracks
🟢 test_ActsValidateTracks
🟢 test_ActsValidateActsCoreSpacePoints
🟢 test_ActsValidateActsSpacePoints
🟢 test_ActsValidateSeeds
🟢 test_ActsValidateOrthogonalSeeds
🟢 test_ActsValidateClusters
🟢 test_ActsPersistifyEDM
🟢 test_ActsGSFRefitting
🟢 test_ActsKfRefitting
🟢 test_ActsExtrapolationAlgTest
🟢 test_ActsITkTest
🟢 run_unit_tests

@acts-project-service acts-project-service added Breaks Athena build This PR breaks the Athena build Fails Athena tests This PR causes a failure in the Athena tests labels Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CarloVarni CarloVarni CarloVarni left review comments

@paulgessinger paulgessinger paulgessinger approved these changes

@andiwand andiwand andiwand left review comments

Assignees
No one assigned
Labels
Breaks Athena build This PR breaks the Athena build Component - Core Affects the Core module Component - Examples Affects the Examples module Event Data Model Fails Athena tests This PR causes a failure in the Athena tests Seeding
Projects
None yet
Milestone
v37.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@stephenswat @acts-project-service @andiwand @paulgessinger @CarloVarni @AJPfleger