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

Add Ambiguity Resolution Performance Writer #521

Conversation

SylvainJoube
Copy link
Contributor

@SylvainJoube SylvainJoube commented Feb 21, 2024

Performance writer for the Greedy Ambiguity Resolution Algorithm, to be used with ROOT visualizer.

I started the implementation by copying and modifying the finding_performance_writer.hpp/cpp files, adapting them to the greedy ambiguity resolution algorithm.

@beomki-yeo
Copy link
Contributor

I am not very inclined to add new writers
Is there any notable difference between ambiguity_resolver_performance_writer and finding_performance_writer?
If not, I think we can just make another finding_performance_writer for AR track states output.

At some point, seeding_performance_writer will also be merged into finding_performance_writer for better maintanence.

@SylvainJoube
Copy link
Contributor Author

SylvainJoube commented Feb 23, 2024

I understand, the less redundant code the better! I will try to merge the ambiguity_resolver_performance_writer into the finding_performance_writer. The only difference lies in how we obtain the std::vector<measurement> measurements for each track, that's all.

@SylvainJoube SylvainJoube force-pushed the greedy-ambiguity-resolution-performance branch from ffa542e to b3f8cf2 Compare February 23, 2024 14:34
@SylvainJoube
Copy link
Contributor Author

If you find the modification appropriate, I will rebase this branch with the main branch later today.

Copy link
Member

@krasznaa krasznaa left a comment

Choose a reason for hiding this comment

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

Formally/technically I'm okay with these changes. But I'll wait for @beomki-yeo's decision... 🤔

Copy link
Contributor

@beomki-yeo beomki-yeo 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 to me as well

@krasznaa krasznaa force-pushed the greedy-ambiguity-resolution-performance branch from b3f8cf2 to 0386d73 Compare March 4, 2024 14:08
@krasznaa krasznaa force-pushed the greedy-ambiguity-resolution-performance branch from 0386d73 to 8c616af Compare March 5, 2024 18:38
@krasznaa krasznaa merged commit 5f0fa34 into acts-project:main Mar 5, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants