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

Remove the colorFlow member from the MCParticle #389

Merged
merged 4 commits into from
Jan 10, 2025

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Dec 17, 2024

BEGINRELEASENOTES

  • Remove the colorFlow member variable from the MCParticle since it is effectively unused (and should remain that way as there is no really correct way to use it without baking in generator specific assumptions).
    • This is a backwards compatible change w.r.t. readability of existing files (ROOT schema evolution handles this)
    • This is a breaking change for the API as {get,set}ColorFlow are removed from the MCParticle.
  • Remove the now unused edm4hep::Vector2i from the components

ENDRELEASENOTES

This is a follow up on #237 and another discussion with generator / HepMC3 authors. The only place where colorFlow is used in the iLCSoft / LCIO world is the TrueJet processor. However, also in there it has remained at the idea stage of an implementation, and is not actually used.

Since this is effectively generator internal information and EDM4hep is not the format to exchange data between generators we decided to remove this.

@wdconinc
Copy link
Contributor

  • do you actually use it in EDM4eic somewhere?

We do not use Vector2i or colorFlow (i.e. we use Vector2i only to set colorFlow to some default value when we have to create MCParticles). We can get rid of it and work around it with preprocessor directives on the podio version.

@tmadlener
Copy link
Contributor Author

We should (hopefully) also export enough information through the pre-processor for EDM4hep to check on that directly (instead of making a proxy check via podio).

@wdconinc
Copy link
Contributor

We should (hopefully) also export enough information through the pre-processor for EDM4hep to check on that directly (instead of making a proxy check via podio).

Yes, sorry, we do indeed us EDM4HEP_VERSION_MAJOR etc, no need to go through podio (though schema_version could be useful to expose).

@tmadlener
Copy link
Contributor Author

Ah we don't do that yet via the pre-processor, but it's available as a constexpr static variable.

#include <edm4hep/DatamodelDefinition.h>

if constexpr (edm4hep::meta::schemaVersion > 2) {
  // do something
}

s/edm4hep/your-data-model/g will give you the same for your-data-model in case it is ever needed.

@tmadlener tmadlener force-pushed the mcparticle-rm-colorflow branch from 3031db0 to 3fd0daf Compare December 18, 2024 09:39
test/test_EDM4hepFile.py Outdated Show resolved Hide resolved
@andresailer andresailer mentioned this pull request Dec 18, 2024
2 tasks
@tmadlener tmadlener force-pushed the mcparticle-rm-colorflow branch from 99168c8 to 607629b Compare December 19, 2024 12:08
jmcarcell added a commit to jmcarcell/DD4hep that referenced this pull request Jan 3, 2025
jmcarcell added a commit to key4hep/k4geo that referenced this pull request Jan 3, 2025
jmcarcell added a commit to key4hep/k4geo that referenced this pull request Jan 3, 2025
andresailer pushed a commit to AIDASoft/DD4hep that referenced this pull request Jan 9, 2025
@jmcarcell
Copy link
Member

This should be ready, the downstream build https://github.com/key4hep/EDM4hep/actions/runs/12412376830/job/35417731197?pr=389 doesn't show any issues related to this.

@tmadlener tmadlener force-pushed the mcparticle-rm-colorflow branch from 607629b to 3bf1ab1 Compare January 10, 2025 10:59
@tmadlener tmadlener merged commit 1c60905 into key4hep:main Jan 10, 2025
8 of 10 checks passed
MarkusFrankATcernch pushed a commit to MarkusFrankATcernch/DD4hep that referenced this pull request Jan 15, 2025
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