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

Refactor II #30

Merged
merged 17 commits into from
Jun 6, 2024
Merged

Refactor II #30

merged 17 commits into from
Jun 6, 2024

Conversation

Zehvogel
Copy link
Collaborator

Next batch in the refactoring series. Mostly just moving (almost) all the definitions from the main steering file to sub files and then loading them with the sequenceLoader copied from iLCSoft/ILDConfig#137. The latter can be removed once we have it (or something similar) in key4hep proper.

BEGINRELEASENOTES

  • Move most parts of the reconstruction steering into sub files loaded by the sequenceLoader

ENDRELEASENOTES

There will be a third batch of patches to add some more configuration flags and refactor the output definitions soon ™️

@Zehvogel
Copy link
Collaborator Author

@tmadlener can you consent to the py_utils.py I stole from you being included here under apache2 instead of gpl3 as in ILDConfig? :)

@tmadlener
Copy link
Contributor

@tmadlener can you consent to the py_utils.py I stole from you being included here under apache2 instead of gpl3 as in ILDConfig? :)

Sure, no problem :) As I have already alluded to in the original pull request for k4FWCore, I am not yet entirely happy with the SequenceLoader I came up with because it has a few too many implicit assumptions. Once, I have a few more spare cycles I would like to do a proper AlgorithmList or something in that direction with similar functionality. However, from the SequenceLoader to that the refactoring should be minimal (and only be necessary for the "main" config and not the individual modules).

@Zehvogel Zehvogel force-pushed the refactor branch 2 times, most recently from 0d2ee32 to 6bb7fcb Compare May 17, 2024 07:59
@Zehvogel
Copy link
Collaborator Author

I am not sure why the edm4hep input test does not fail on my machine so it is a bit ugly to debug. It looks related to the PID stuff... @tmadlener maybe you have a good guess what happens? :)

@tmadlener
Copy link
Contributor

PodioOutput ERROR Could not write event, because the following collections are not present: RefinedVertexJets_PID_RefinedVertex

Looks like some whatever produces the RefinedVertexJets only produces the empty collection (which obviously) will not have a PIDHandler attached, so the conversion then fails to create the correpsonding ParticleIDCollection on the EDM4hep side.

@Zehvogel
Copy link
Collaborator Author

Looks like some whatever produces the RefinedVertexJets only produces the empty collection (which obviously) will not have a PIDHandler attached, so the conversion then fails to create the correpsonding ParticleIDCollection on the EDM4hep side.

That sounds likely, yes. :(
Do you have an idea for a fix?

@tmadlener
Copy link
Contributor

The Processor that produces this has to be fixed. It needs to unconditionally add a PIDHandler to the collection. Currently, it probably just produces the empty collection but doesn't attach that.

@Zehvogel
Copy link
Collaborator Author

Oh great thankfully the issue for this already exists...
lcfiplus/LCFIPlus#69

@andresailer andresailer merged commit 0ab84f6 into key4hep:main Jun 6, 2024
3 of 5 checks passed
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