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 Delphes isolation variable and ParticleFlowCandidates to edm4hep output #107

Merged
merged 40 commits into from
Apr 21, 2023

Conversation

bistapf
Copy link
Contributor

@bistapf bistapf commented Apr 19, 2023

BEGINRELEASENOTES

  • Add the Isolation Variable as computed by Delphes to the edm4hep conversion output as a UserDataCollection
  • Add ParticleFlowCandidates as possible collection to converter

ENDRELEASENOTES

Birgit Stapf and others added 30 commits January 24, 2023 14:19
…oading weight from delphes not yet functional
@bistapf bistapf changed the title Add iso var Add Delphes isolation variable and ParticleFlowCandidates to edm4hep output Apr 19, 2023
Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

Thanks for this as well :)

As a general comment could you remove the debug output that was introduced during development? There are also a few more comments / questions inline.

One "big picture" question: Currently the ParticleFlow candidates do not get a relation to an MC particle (like other ReconstructedParticles do). Since they are a somewhat Delphes internal thing, I am not sure if we should actually add one, or whether we leave them like it is.

Finally, could you also add documentation about these new features to doc/output_config.md.

@bistapf
Copy link
Contributor Author

bistapf commented Apr 20, 2023

Thanks for your feedback, Thomas :)

I have implemented the suggested changes, updated the documentation and in particular added a statement there that currently the ParticleFlowCandidates do not have any MC associations.

@@ -41,6 +43,9 @@ the assumption that this is a non-overlapping list of particles. It is the users
responsibility to make sure that this is the case. (See [known
issues](#known-issues)).

`ParticleFlowCandidate` collections from Delphes can also be stored as a `ReconstructedParticleCollection` in the output, but they currently have no associations to the generated particles.
Copy link
Contributor

Choose a reason for hiding this comment

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

They will also not be added to the global ReconstructedParticle collection as that would lead to double counting (given that the ParticleFlowCandidate is a somewhat Delphes internal class)

| n/a | `MCRecoParticleAssociation` |

All Delphes classes that are not listed here are currently not converted.

## EventHeader Collection
The `EventHeader` collection is used to store information from the Delphes `Event` classes. It contains one element for the `eventNumber` and one for the `weight`. This is currently only implemented for the `DelphesPythia8Reader`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, the eventNumber and weight are stored in a single edm4hep::EventHeader. The way it is currently written it could also be interpreted as if there are multiple of these with partial information each.

* The settings steering the output of the DelphesEDM4HepConverter. Default
* values correspond to the ones found in delphes example cards.
*/
* The settings steering the output of the DelphesEDM4HepConverter. Default
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these re-indentations really necessary? I suppose they are added automatically be by clang-format?

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.

2 participants