-
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: Add python vertex example + test #1028
feat: Add python vertex example + test #1028
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1028 +/- ##
==========================================
- Coverage 48.62% 48.62% -0.01%
==========================================
Files 341 341
Lines 17511 17512 +1
Branches 8244 8245 +1
==========================================
Hits 8515 8515
- Misses 3232 3233 +1
Partials 5764 5764
Continue to review full report at Codecov.
|
06e9eea
to
a38d0b7
Compare
This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs. |
@Corentin-Allaire this is what I was talking about this morning. |
@paulgessinger I can cross-check it with the how to and try to run it. I guess for now that will be good enough until either someone with more vertexing knowledge can look at it or someone need to run it and can provide feedback. |
@Corentin-Allaire sounds good! |
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.
So I have a few comments on the PR. I am obviously not a big vertexing expert but I think this can go in.
I updated this. Unfortunately, I got an error when actually increasing the number of events the test runs on. In the AMVF variant with particle input, on the 59th event, AMVF complains that an empty input is provided. IVF does not seem to have a problem with that event it seems.
There is still the case of the summary writer complaining that the truth particles are mismatched w.r.t. to the reconstructed tracks. I guess this expected if we don't reconstruct a truth particle?
|
Ah yeah, the cases I mentioned in the previous comment is actually a WARNING, which we disallow in the CI. Any opinions on what we should do here? |
I created #1091 to track the issues that remain with the code added here. |
so this doesn't have to cause an error in the AMVF anymore.
This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs. |
You should resolved the conversations. |
Ok I think it's all green now. Can you approve again if you're happy, @Corentin-Allaire? |
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.
Don't forget to close the last issue !
This test tries to model all the vertexing combinations, where I noticed that some of them don't seem to produce outputs that make sense to me. It might be that some combinations are not supported / never tested, and I added a warning to the example script if such a combination is configured.
Right now this is still WIP because it's rebased on #1027 since it uses the truth tracking for the test
@baschlag could you help and take a look at this to see if this implementation makes sense?