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: Update EventRecording to remove disconnected vertices and particles #912

Merged
merged 6 commits into from
Sep 1, 2021
Merged

Conversation

andriish
Copy link
Contributor

@andriish andriish commented Aug 3, 2021

Prevent problems similar to #877

Prevent problems similar to #877
@andriish andriish changed the title Update EventRecording.cpp fix: Update EventRecording.cpp Aug 3, 2021
@codecov
Copy link

codecov bot commented Aug 3, 2021

Codecov Report

Merging #912 (a567bce) into main (c07fcd7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #912   +/-   ##
=======================================
  Coverage   48.48%   48.48%           
=======================================
  Files         334      334           
  Lines       17091    17091           
  Branches     8081     8081           
=======================================
  Hits         8287     8287           
  Misses       3099     3099           
  Partials     5705     5705           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c07fcd7...a567bce. Read the comment docs.

@paulgessinger
Copy link
Member

This does not seem to compile

@paulgessinger
Copy link
Member

@andriish can I close this then?

@andriish
Copy link
Contributor Author

andriish commented Aug 5, 2021

I suspect there might be the same problem as in #877.

A simple for will not work, as there might be a case of e.g. two vertices and one particle connected together but disconnected from all other particles/vertices in the event.

VertexA ---ParticleA-->VerxexB

So after one for loop the event will still contain disconnected particles/vertices.

@robertlangenberg
Copy link
Contributor

Hi @andriish could you give a bit more explanation what you want to fix here and get the CI to go green? Thanks!

@andriish
Copy link
Contributor Author

Hi @robertlangenberg ,

what you want to fix

exactly the behaviour of #877.

Best regards,

Andrii

@andriish
Copy link
Contributor Author

Hi @paulgessinger , @robertlangenberg ,

any progress with this MR?
I'm not using the acts by myself, just helped with some issues that were related to the work of @FabianKlimpel , so I would be grateful if this MR would be resolved is some way.

Best regards,

Andrii

@paulgessinger
Copy link
Member

Hey @andriish,

we still do not quite understand how this PR relates to #877 and #911. Is this changing the EventRecording so that problematic vertices do not get appended at all, while #911 removes them at the end? If so, why do we need to do both?

@andriish
Copy link
Contributor Author

Hi @paulgessinger ,

  • the current logic of the "cleaning" in the very end is:

    go through the event ONCE and remove

    • the vertices w/o outgoing particles.

But it is not enough to remove only the vertices w/o outgoing particles to ensure the event has a tree-like structure.

  • so the proposed logic is:

    loop over particles and vertices until there will be no

    • particles w/o prod. vertex
    • vertices w/o outgoing particles

The "proposed logic" cleans up more "broken" cases in the event structure (e.g. similar to #877) than the current version and assures creation of an event with a tree-like structure that can be then correctly read back.

Of course, not all events will contain particles w/o prod. vertex or vertices w/o outgoing particles. And for some events the current version of code will work fine, but it is better to be on the safe sie, as for me.

Best regards,

Andrii

P.S. An example of "bad" event structure that would not be cured by the current logic is

ParticleA has some prod_vertex VertexX and end_vertex VertexY
ParticleB has no prod_vertex but has end_vertex VertexY
VertexY has no outgoing particles

The current version of code would just remove the VertexY, but completely disconnected particleB would remain in the event.

@asalzburger
Copy link
Contributor

I started cleaning out the Geant4 examples a bit more, we have several files that are now called the same, e.g. SteppingAction.h/cpp, etc.

What I suggest (PR in work) is:

  • Material*Actions - for recording/writing the material
  • Simulation*Actions - for a very simplified/limited Geant4 simulation (mainly to be used for Fatras validation)
  • EventRecording*Actions - for the interaction recording & parameterisation stuff

@paulgessinger
Copy link
Member

@asalzburger they're called the same but I moved them into separate namespaces in a PR that was merged a few weeks ago.

@paulgessinger paulgessinger changed the title fix: Update EventRecording.cpp fix: Update EventRecording to remove disconnected vertices and particles Aug 31, 2021
@paulgessinger
Copy link
Member

I updated the format of the EventRecording.cpp file.

@paulgessinger paulgessinger added this to the next milestone Aug 31, 2021
@andriish
Copy link
Contributor Author

andriish commented Sep 1, 2021

Thanks, @paulgessinger!

If there will be any issues related to HepMC, ping me.

Best regards,

Andrii

@paulgessinger paulgessinger modified the milestones: next, v13.0.0 Sep 21, 2021
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.

None yet

4 participants