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

fix: Unit test hough transform and fix identifier bug #3332

Merged
merged 17 commits into from
Jul 3, 2024

Conversation

dimitra97
Copy link
Contributor

@dimitra97 dimitra97 commented Jun 26, 2024

This PR introduces a Unit Test for the Hough transform by taking hardcoded the data for the MuonSimHit and the DriftCircles (since we just want to test the TransformUtils and not the readers). We check one station and we pick also the corresponding sim hit for this station from the csv files.

Also it fixes a bug with the bit shift operation in the MuonSimHit.hpp because we observed a problem (with @janheinri during the tracking workshop) when the stationEta was negative so we cast it to unsigned int
Tagging people that may be interested: @goblirsc @junggjo9 @janheinri (if you also want to review :) )

Core/include/Acts/Seeding/HoughVectors.hpp Outdated Show resolved Hide resolved
Tests/UnitTests/Core/Seeding/CMakeLists.txt Outdated Show resolved Hide resolved
Tests/UnitTests/Core/Seeding/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@goblirsc goblirsc left a comment

Choose a reason for hiding this comment

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

Hi,

thanks a lot @dimitra97 @janheinri !
I added a few comments from my end, including a proposal for how we can avoid the ActsExamples dependency.

Cheers, Max

Tests/UnitTests/Core/Seeding/HoughTransformTest.cpp Outdated Show resolved Hide resolved
Tests/UnitTests/Core/Seeding/HoughTransformTest.cpp Outdated Show resolved Hide resolved
Tests/UnitTests/Core/Seeding/HoughTransformTest.cpp Outdated Show resolved Hide resolved
Tests/UnitTests/Core/Seeding/HoughTransformTest.cpp Outdated Show resolved Hide resolved
Tests/UnitTests/Core/Seeding/HoughTransformTest.cpp Outdated Show resolved Hide resolved
Tests/UnitTests/Core/Seeding/HoughTransformTest.cpp Outdated Show resolved Hide resolved
Tests/UnitTests/Core/Seeding/HoughTransformTest.cpp Outdated Show resolved Hide resolved
@dimitra97
Copy link
Contributor Author

Hi,

thanks a lot @dimitra97 @janheinri ! I added a few comments from my end, including a proposal for how we can avoid the ActsExamples dependency.

Cheers, Max

Hello Max, ok I if we want to discard ActsExamples dependency I will follow the comments and make the changes!
Thanks,
Dimitra

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.

I like it!

janheinri
janheinri previously approved these changes Jun 28, 2024
Copy link

@janheinri janheinri left a comment

Choose a reason for hiding this comment

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

I don't think I have anything valuable to add which wasn't mentioned already

@acts-policybot acts-policybot bot dismissed janheinri’s stale review June 28, 2024 11:55

Invalidated by push of 3e8cd4e

@andiwand andiwand added this to the next milestone Jul 2, 2024
@andiwand andiwand changed the title feat: testing the hough transform in a unittest and fix a bug fix: Unit test hough transform and fix identifier bug Jul 2, 2024
andiwand
andiwand previously approved these changes Jul 2, 2024
andiwand
andiwand previously approved these changes Jul 2, 2024
Copy link

sonarcloud bot commented Jul 3, 2024

@kodiakhq kodiakhq bot merged commit a283841 into acts-project:main Jul 3, 2024
52 checks passed
@github-actions github-actions bot removed the automerge label Jul 3, 2024
@paulgessinger paulgessinger modified the milestones: next, v36.0.0 Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants