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

Transfer EventHeader information to LCIO event #111

Merged
merged 6 commits into from
May 5, 2023

Conversation

Zehvogel
Copy link
Contributor

@Zehvogel Zehvogel commented Mar 28, 2023

BEGINRELEASENOTES

ENDRELEASENOTES

There are still some TODOs in here where I was not sure what the best option to warn the user would be.

Depends on key4hep/k4EDM4hep2LcioConv#13

Copy link
Collaborator

@andresailer andresailer left a comment

Choose a reason for hiding this comment

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

nicely spotted the problem!

k4MarlinWrapper/src/components/EDM4hep2Lcio.cpp Outdated Show resolved Hide resolved
k4MarlinWrapper/src/components/EDM4hep2Lcio.cpp Outdated Show resolved Hide resolved
k4MarlinWrapper/src/components/EDM4hep2Lcio.cpp Outdated Show resolved Hide resolved
k4MarlinWrapper/src/components/EDM4hep2Lcio.cpp Outdated Show resolved Hide resolved
@tmadlener
Copy link
Contributor

Should we consider moving the actual "conversion" to k4EDM4hep2LCIOConv and simply put the necessary Gaudi wrapper code in here?

@andresailer
Copy link
Collaborator

Should we consider moving the actual "conversion" to k4EDM4hep2LCIOConv and simply put the necessary Gaudi wrapper code in here?

Yes, you are correct, we should put the "conversion" in k4EDM4hep2LCIOConv and call it from here.

@Zehvogel
Copy link
Contributor Author

could you specificy which part of the "conversion" I should move? Should the function in k4EDM4hep2LCIOConv return the 4 values or also set them in the LCIO event? The latter would be contrary to how the other functions are while the former seems overly complicated?

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.

The "conversion" in this case is effectively the 4 lines of setting information in the LCEvent. The interface in k4EDM4hep2LcioConv should probably looks something like:

void setEventHeaderInfo(lcio::LCEventImpl* evt, const edm4hep::EventHeaderCollection* const evtHeaderColl);

so that the k4MarlinWrapper side of this would effectively boil down to getting the handle and passing it on.

k4MarlinWrapper/src/components/EDM4hep2Lcio.cpp Outdated Show resolved Hide resolved
@Zehvogel
Copy link
Contributor Author

I made the suggested changes and also opened key4hep/k4EDM4hep2LcioConv#13

@Zehvogel Zehvogel requested a review from tmadlener April 18, 2023 13:59
@tmadlener
Copy link
Contributor

Unrelated clang-format issues should be fixed with #112

@Zehvogel
Copy link
Contributor Author

I moved the size check + error message to this side now and got rid of the unrelated format changes

@Zehvogel Zehvogel requested a review from tmadlener April 19, 2023 06:43
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. Looks good to me. I would just like to have at least one successful build in CI before merging this.

@Zehvogel
Copy link
Contributor Author

Zehvogel commented May 4, 2023

Can we get this merged? It builds successfully in the nightlies and I think the failing test is also failing on master and caused by #113.

@andresailer andresailer merged commit 5006557 into key4hep:master May 5, 2023
@Zehvogel Zehvogel deleted the convert-header branch May 5, 2023 10:42
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.

Issues when running CLIC reconstruction on EDM4Hep input
3 participants