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

feat: Add common hough transform components to core #2836

Merged
merged 34 commits into from
Jan 23, 2024

Conversation

goblirsc
Copy link
Contributor

This PR refactors the logic originally put into ACTS by @jahreda for the example hough transform algorithm.
Migrate suitable for common usage to core.
Fix a few bugs in the process and improve execution efficiency.
Add an additional example algorithm implementing an ATLAS muon spectrometer hough transform to test the new logic.
CC @junggjo9 @noemina @dimitra97

@paulgessinger paulgessinger added this to the next milestone Dec 15, 2023
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.

Looks good overall. For the types that go into core, we have slightly more strict naming rules. I added a few comments on this. At the Examples level, this is much more loosely enforced, because it's not shipped.

Core/include/Acts/Seeding/HoughTransformUtils.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/HoughTransformUtils.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/HoughTransformUtils.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/HoughTransformUtils.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/HoughTransformUtils.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/HoughVectors.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/HoughVectors.hpp Outdated Show resolved Hide resolved
Examples/Io/Csv/src/CsvSimHitReader.cpp Outdated Show resolved Hide resolved
Examples/Io/Csv/src/CsvSimHitReader.cpp Outdated Show resolved Hide resolved
Examples/Scripts/Python/muon_hough.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented Dec 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (21d6c53) 48.98% compared to head (5160454) 48.99%.

❗ Current head 5160454 differs from pull request most recent head 8501e7b. Consider uploading reports for the commit 8501e7b to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2836      +/-   ##
==========================================
+ Coverage   48.98%   48.99%   +0.01%     
==========================================
  Files         494      492       -2     
  Lines       28742    28527     -215     
  Branches    13606    13485     -121     
==========================================
- Hits        14078    13978     -100     
+ Misses       4854     4814      -40     
+ Partials     9810     9735      -75     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@goblirsc
Copy link
Contributor Author

Dear @paulgessinger, many thanks for the first look at this MR! One Q, do I need to do a manual squash & rebase, or will this be done later?

@paulgessinger
Copy link
Member

This happens at the end, no need to do it manually!

@asalzburger
Copy link
Contributor

uuups, I think I did a merge fu ...

@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Jan 10, 2024
@paulgessinger paulgessinger merged commit 785654b into acts-project:main Jan 23, 2024
33 of 48 checks passed
@acts-project-service
Copy link
Collaborator

acts-project-service commented Jan 23, 2024

✅ Athena integration test results [785654b]

✅ All tests successful

status job report
🟢 run_unit_tests
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsEFTrackFit.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsBenchmarkWithSpot.sh 8 100
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsWorkflow.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateAmbiguityResolution.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateResolvedTracks.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateTracks.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateActsCoreSpacePoints.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateActsSpacePoints.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateSeeds.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateOrthogonalSeeds.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsValidateClusters.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsPersistifyEDM.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/test/ActsGSFRefitting.sh
🟢 run_ci_tests: ../athena/Tracking/Acts/ActsConfig/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
🟢 run_art_test: test_data18_13TeV_1000evt
🟢 run_art_test: test_ttbarPU40_reco

@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Jan 23, 2024
@paulgessinger
Copy link
Member

Athena failure is unrelated to changes, ATLAS / CERN is having infrastructure issues.

@paulgessinger paulgessinger modified the milestones: next, v32.1.0 Feb 2, 2024
LaraCalic pushed a commit to LaraCalic/acts that referenced this pull request Feb 10, 2024
This PR refactors the logic originally put into ACTS by @jahreda for the
example hough transform algorithm.
Migrate suitable for common usage to core. 
Fix a few bugs in the process and improve execution efficiency.
Add an additional example algorithm implementing an ATLAS muon
spectrometer hough transform to test the new logic.

---------

Co-authored-by: jahreda <57440432+jahreda@users.noreply.github.com>
Co-authored-by: Riley Xu
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 Component - Examples Affects the Examples module Event Data Model Fails Athena tests This PR causes a failure in the Athena tests Infrastructure Changes to build tools, continous integration, ... Seeding Track Finding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants