-
Notifications
You must be signed in to change notification settings - Fork 157
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: Initial version of ATLAS-based FTF seeding #2227
Conversation
let's see if this runs through 🤞 |
@andiwand Great, thank you so much for all your help! |
It has the "This branch is out-of-date with the base branch" warning should I update the branch or will I cause it to reset ? |
that should not be a problem. but |
I need to add @Rosie-Hasan to the relevant GitHub team for that to work. EDIT: Done now, but you'll have to accept the team invitation before it's effective. |
Hmm codecov is failing on this @paulgessinger |
Well it does it's job 😄 |
I believe this merge fails the Building it locally also gave me a bunch of warnings. |
I thought I checked and they were green earlier. I'll fix them tomorrow. |
I started fixing this here #2565 |
clang tidy fixes after #2227
memset(&m_Cx[0][0], 0, sizeof(m_Cx)); | ||
memset(&m_Cy[0][0], 0, sizeof(m_Cy)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a trial for commenting on already-merged code as a prompt for future improvement.
Please replace memset
by std::fill
. memset
here is undefined behaviour, but works because 0.0f
is represented by 4 '\0'
s. A bit awkwardly, you need to specify the number of elements as sizeof(m_Cx)/sizeof(m_Cx[0][0])
.
If m_Cx
were a std::array
, then it would know its own size and all would be good.
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 ](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
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
First few files needed for FTF seeding. Does not create seeds yet, made to check current progress is compatible.