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

Change the default output name for the reco - mc links #139

Closed
1 task done
tmadlener opened this issue Nov 27, 2024 · 11 comments · Fixed by #140
Closed
1 task done

Change the default output name for the reco - mc links #139

tmadlener opened this issue Nov 27, 2024 · 11 comments · Fixed by #140
Labels
enhancement New feature or request

Comments

@tmadlener
Copy link
Contributor

tmadlener commented Nov 27, 2024

Currently the default output name in the example for the link between Reco and MC particles is slightly misleading regarding the direction of the link. Following the EDM4hep naming convention the name suggests that MC is from and reco is to, when in fact the RecoMCParticleLink defines them in the opposite way.

set RecoMCParticleLinkCollectionName MCRecoAssociations

  • We have to make sure that changing the example here doesn't break any production

@BrieucF just FYI

@tmadlener tmadlener added the enhancement New feature or request label Nov 27, 2024
@tmadlener
Copy link
Contributor Author

@kjvbrt also as a heads up, following this change for FCC would break FCCAnalyses quite heavily, I think.

@bistapf
Copy link
Contributor

bistapf commented Nov 27, 2024

I think only the datasource version of FCCAnalyses explicitly uses the link, while the old style still does the manual index hopping anyway - or am I mistaken, @kjvbrt ? E.g. code that relies on it: https://github.com/HEP-FCC/FCCAnalyses/blob/b9b84221837da8868158f5592b48a9af69f0f6e3/analyzers/dataframe/src/ReconstructedParticleSource.cc#L22

Since we have started a production for FCC-hh recently it wouldn't be super ideal to make this renaming. However we don't currently use the links for FCC-hh analyses, but once this is propagated to a release, will files produced with an older version before the name change still be readable as long as the RecoMCParticleLink is not used?

Given that the FCC-hh production currently relies on nightlies at this stage (mainly due to #136) things are a bit complicated though. I think it may be fine to change the name, but then as soon as possible, so it's in the next stable release that would officially be used for the production then. If files pre-namechange become unreadable then though, it would mean reproducing some samples for FCC-hh.

@BrieucF
Copy link

BrieucF commented Nov 27, 2024

Thanks @tmadlener for opening the issue and @bistapf for checking already the impact it would have.

I also checked whether there were other definitions of EDM4HepOutput in FCC-Config (there is none). In EventProducer, there is nevertheless the possibility to use a custom EDM4HepOutput tcl file: EventProducer.

I will investigate further and let you know

@BrieucF
Copy link

BrieucF commented Nov 27, 2024

Edit to the above: there is one in FCC-config (https://github.com/HEP-FCC/FCC-config/blob/winter2023/FCCee/Delphes/edm4hep_IDEA.tcl) but it is in the winter2023 branch which is used with a fixed stable release using the 'old' associations. Everything is fine in this regard.

So the last word goes to you @bistapf !

@kjvbrt
Copy link
Contributor

kjvbrt commented Nov 29, 2024

Hi, the change will affect mostly user code (analysis scripts) not really any production code and it does not matter if users use FCCAnalyses with datasource or not.
As long as the name is consistent among the samples in the campaign, this should be fine.

@BrieucF
Copy link

BrieucF commented Dec 3, 2024

Thanks Juraj, @bistapf, can we proceed?

@bistapf
Copy link
Contributor

bistapf commented Dec 3, 2024

Hi @BrieucF, all. Thanks a lot for the follow-up. Given that for FCC-hh I actually do use a custom edm4hep output config file already, I think there isn't any technial issue from my side. For the record, we agreed for now to keep the old name in the FCC-hh v06 production, and once we move to a new production we will update the name also.

TL,DR: Yes, please go ahead and merge.

@jmcarcell
Copy link
Member

jmcarcell commented Dec 4, 2024

What about this line?

set MCRecoAssociationCollectionName RecoMCLink

Should it also be changed (the part on the left)? There is already a deprecation warning for this name from a few months ago: https://github.com/key4hep/k4SimDelphes/blob/main/converter/include/k4SimDelphes/DelphesEDM4HepOutputConfiguration.h#L156

@tmadlener
Copy link
Contributor Author

tmadlener commented Dec 5, 2024

Ah, damnit. Good catch. Didn't even think about that. That should definitely also be fixed. PR in the making.

edit: see #141

@jmcarcell
Copy link
Member

What about the warning, should we turn it into an error or keep it there for cards that still use the old name?

@tmadlener
Copy link
Contributor Author

I would keep it as a warning for now and make it an error after the next tag only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants