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

refactor: name change of FTF -> GBTS seeding #2853

Merged
merged 14 commits into from
Jan 9, 2024

Conversation

Rosie-Hasan
Copy link
Contributor

@Rosie-Hasan Rosie-Hasan commented Dec 21, 2023

Change naming of new seeding algorithm from FTF (FastTrackFinder) to GBTS (Graph Based Track Seeding) as this is a clearer desription of the algorithm. Name changes disccuessed in issue #2831 .
Code implemented in PRs #2227 #2726 and more details given presented at ACTS-ITK presention .

Name changes:
SeedFinderFTF -> SeedFinderGBTS
GNN_DataStorage -> GBTSDataStorage
GNN_Geometry -> GBTSGeometry
SeedingFTFAlgorithm -> GBTSSeedingAlgorithm
FasTrackConnector-> GBTSConnector
TrigBase -> GBTSBase
full_chain_itk_FTF.py -> full_chain_itk_GBTS.py

@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module Seeding Track Finding labels Dec 21, 2023
@Rosie-Hasan Rosie-Hasan changed the title refactor: name change of FTF -> GBTS seeding Draft refactor: name change of FTF -> GBTS seeding Dec 21, 2023
@Rosie-Hasan Rosie-Hasan changed the title Draft refactor: name change of FTF -> GBTS seeding refactor: name change of FTF -> GBTS seeding Dec 21, 2023
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (82c0c80) 48.83% compared to head (1e72a53) 48.83%.

Files Patch % Lines
Core/src/TrackFinding/GbtsConnector.cpp 0.00% 16 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2853   +/-   ##
=======================================
  Coverage   48.83%   48.83%           
=======================================
  Files         486      486           
  Lines       28175    28175           
  Branches    13285    13285           
=======================================
  Hits        13760    13760           
  Misses       4820     4820           
  Partials     9595     9595           

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

@Rosie-Hasan Rosie-Hasan marked this pull request as ready for review December 21, 2023 23:47
@AJPfleger AJPfleger added this to the next milestone Jan 8, 2024
Copy link
Contributor

@AJPfleger AJPfleger left a comment

Choose a reason for hiding this comment

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

Looks already quite fine. I think, there are only(?) 3 things to change:

  • Start variable names with lower case
  • Drop most of the underscores except for m_afterThatNoUnderScores and maybe in the python scripts
  • Change *ipp to .*cpp when you are already renaming files. But this is argueable.

Core/include/Acts/Seeding/GbtsTrackingFilter.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/GbtsTrackingFilter.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/SeedFinderGbts.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/SeedFinderGbts.ipp Outdated Show resolved Hide resolved
@Rosie-Hasan
Copy link
Contributor Author

@AJPfleger Thank you for your help, I am making the name changes for the variables. For the ipp -> cpp I just wanted to double check as all the other files in core seeding are ipp/hpp

@AJPfleger
Copy link
Contributor

For the ipp -> cpp I just wanted to double check as all the other files in core seeding are ipp/hpp
I see, good spot. Then let's keep them as they are :)

Copy link
Contributor

@AJPfleger AJPfleger left a comment

Choose a reason for hiding this comment

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

I think after these changes we are ready to go.

There are still a few other variables that need to be renamed, but they are not really in the scope of this PR. Since you planned to refactor/modernise the code code, some of them might even vanish. So no need to put too much thinking into it right now.

Core/include/Acts/Seeding/GbtsDataStorage.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/SeedFinderGbts.ipp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/SeedFinderGbts.ipp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/SeedFinderGbts.ipp Outdated Show resolved Hide resolved
Core/include/Acts/Seeding/SeedFinderGbts.ipp Outdated Show resolved Hide resolved
Examples/Python/src/TrackFinding.cpp Outdated Show resolved Hide resolved
Rosie-Hasan and others added 3 commits January 9, 2024 13:31
Co-authored-by: Alexander J. Pfleger <70842573+AJPfleger@users.noreply.github.com>
@kodiakhq kodiakhq bot merged commit 53bcd74 into acts-project:main Jan 9, 2024
54 checks passed
@github-actions github-actions bot removed the automerge label Jan 9, 2024
@acts-project-service
Copy link
Collaborator

acts-project-service commented Jan 9, 2024

✅ Athena integration test results [53bcd74]

✅ All tests successful

status job report
🟢 run_unit_tests
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsBenchmarkWithSpot.sh 8 100
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsWorkflow.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateAmbiguityResolution.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateResolvedTracks.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/ActsGSFRefitting.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
🟢 run_art_test: test_data18_13TeV_1000evt
🟢 run_art_test: test_ttbarPU40_reco

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Jan 9, 2024
@paulgessinger paulgessinger modified the milestones: next, v32.0.0 Jan 19, 2024
@paulgessinger paulgessinger removed the Breaks Athena build This PR breaks the Athena build label Jan 23, 2024
LaraCalic pushed a commit to LaraCalic/acts that referenced this pull request Feb 10, 2024
Change naming of new seeding algorithm from FTF (FastTrackFinder) to GBTS (Graph Based Track Seeding) as this is a clearer desription of the algorithm. Name changes disccuessed in issue acts-project#2831 .  
Code implemented in PRs acts-project#2227  acts-project#2726  and more details given presented at [ACTS-ITK presention ](https://indico.cern.ch/event/1321208/#20-update-on-ftf-implementatio).

**Name changes**: 
SeedFinderFTF -> SeedFinderGBTS
GNN_DataStorage -> GBTSDataStorage
GNN_Geometry -> GBTSGeometry
SeedingFTFAlgorithm -> GBTSSeedingAlgorithm 
FasTrackConnector-> GBTSConnector
TrigBase -> GBTSBase
full_chain_itk_FTF.py -> full_chain_itk_GBTS.py
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 Seeding Track Finding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants