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 fitting performance writer by changing the measurement comparator #754

Merged
merged 1 commit into from
Oct 30, 2024

Conversation

beomki-yeo
Copy link
Contributor

@beomki-yeo beomki-yeo commented Oct 26, 2024

The measurement comparison operator with float_epsilon is causing a bug in event_data. When the difference between the new measurement and one in the map is bigger than float_epsilon, the measurement is not added to the map.
(You can find the simple code that reproduces this behavior: https://godbolt.org/z/3hP9P5Eo9)

I fixed the issue by removing the float_epsilon. Also put at instead of [] wherever possible to make sure that there is the measurement in the map

@beomki-yeo beomki-yeo changed the title Fix fitting writer Fix fitting performance writer by changing the measurement comparator Oct 29, 2024
@beomki-yeo beomki-yeo force-pushed the fix-fit-writer branch 2 times, most recently from f18dfb1 to 7edadd5 Compare October 29, 2024 16:58
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.

No big objections on my part, just the two technical questions/suggestions...

core/include/traccc/edm/measurement.hpp Outdated Show resolved Hide resolved
performance/src/utils/event_data.cpp Outdated Show resolved Hide resolved
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.

The rest I'm happy with.

And if you push back firmly enough about the whole std::map::insert point, I can drop that as well. Since your code technically already works correctly like this as well. 😉

performance/src/utils/event_data.cpp Outdated Show resolved Hide resolved
Copy link

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.

😄

@beomki-yeo beomki-yeo merged commit ab5a66f into acts-project:main Oct 30, 2024
26 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