-
Notifications
You must be signed in to change notification settings - Fork 52
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
Update event_map for detray detector #562
Conversation
While doing this, could you also extend |
@krasznaa The seeding and track finding performance check are working now (See the results at below). You can change the condition for particle filtering at
Note that Benchmark Log
Track finding efficiency |
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.
I can overlook the discomfort of making the I/O code yet more complicated, since there I see a path for simplification in the future. But I'm really uncomfortable with switching off the clusterization unit tests.
Especially since the CUDA and SYCL tests are still in place. I imagine those would still fail at the moment. 🤔
@@ -23,6 +23,8 @@ | |||
// System include(s). | |||
#include <functional> | |||
|
|||
/* | |||
@FIXME: Change the csv data format for csv |
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 pretty bad. 😦 How come we can't read both cell data types now? Then why does the current main
branch read the new CSV cell files without errors? (Even if one of the columns changed names.)
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.
I don't know. Feel free to fix the PR if you find the reason.
This is what I get when running the test in this PR
[ FAILED ] SparseCclAlgorithm/ConnectedComponentAnalysisTests.Run/trackml_like_0000000028, where GetParam() = (32-byte object <10-FE 29-01 00-00 00-00 70-DB 2B-01 00-00 00-00 E0-51 46-00 00-00 00-00 20-59 46-00 00-00 00-00>, "trackml_like_0000000028") (0 ms)
[ RUN ] SparseCclAlgorithm/ConnectedComponentAnalysisTests.Run/trackml_like_0000000029
unknown file: Failure
C++ exception with description "Missing header column 'measurement_id'" thrown in the test body.
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.
I also don't see the error in the main
branch, but the hit_id
is always zero for ODD data which drops the efficiency to zero as you saw in the mattermost.
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.
Let's keep this one open for the moment. 🤔 I'll try it out shortly from your branch.
Worst case scenario, we make some measurements using this branch as a temporary solution, and then we discuss how to implement all of these things in a slightly more pleasing way. 🤔
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.
Sure
Don't remember what it was about, but it seems overlapping with #692 |
Some breaking changes
test_clsterization_resolution
is disabled now. I don't know exactly why but there is a mismatch between modules fromspacepoint_formation
and truth one. I believe @krasznaa can follow the logics inread_spacepoint
and fix latermapper
tests with tml geometry are deleted. csv geometry is not supported anymore...test_cca
is also disabled because of missing measurement_id header in the old cell csv data format. @stephenswat can follow up this later as well.mapper
is pretty error-prone right now because of the switch betweenmeasurement_id
andhit_id
, and all the fixes that I made are under the assumption that two IDs are identical, which may not be true. Thus, @krasznaa if you find something wrong with the performance, I suggest investigatinghit_map
inmapper
at the first placeIn the near future we will need to wipe out all old data format and stop supporting them in traccc.
mapper
orevent_map
also needs to be rewritten from scratch for better maintenance.