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

Modernize event display example steering #178

Merged
merged 2 commits into from
Apr 11, 2024

Conversation

Zehvogel
Copy link
Contributor

@Zehvogel Zehvogel commented Mar 15, 2024

BEGINRELEASENOTES

  • Modernize event display example

ENDRELEASENOTES

I modernized the event display example a bit and made it able to deal with both lcio and edm4hep files.

The corresponding documentation still needs to be updated. However, it contains a lengthy step by step description of the steering file content that is no longer valid and the only thing I can offer regarding those parts is to delete them 🤷 but before doing so I wanted to gather some feedback :)

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. A few smallish comments inline.

As far as the README is concerned, I would do the following:

  • Remove all the outdated parts that were describing the steps that were necessary to get from the automatically converted on to a working version related to handling of EDM4hep inputs.
  • Remove the explanation of the detailed technical explanation of running things through the MarlinWrapper and rather reference other existing documentation for this
  • Update the section of how this script was obtained originally (mainly rephrasing the last two bullet points to some general information about handling EDM4hep input and replacing InitDD4hep with the GeoSvc)

k4MarlinWrapper/examples/event_display.py Outdated Show resolved Hide resolved
k4MarlinWrapper/examples/event_display.py Outdated Show resolved Hide resolved
k4MarlinWrapper/examples/event_display.py Outdated Show resolved Hide resolved
tmadlener
tmadlener previously approved these changes Mar 27, 2024
k4MarlinWrapper/examples/event_display.py Show resolved Hide resolved
k4MarlinWrapper/examples/event_display.py Show resolved Hide resolved
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.

I think the only thing left to do would be to make the compactFile a command line argument. After that I would merge this (with the EventSelector removed).

@Zehvogel
Copy link
Contributor Author

Zehvogel commented Apr 9, 2024

done (I hope)

@Zehvogel Zehvogel marked this pull request as ready for review April 9, 2024 13:13
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.

Apart from the typo, I think this is good to go.

doc/starterkit/k4MarlinWrapperCLIC/CEDViaWrapper.md Outdated Show resolved Hide resolved
@tmadlener
Copy link
Contributor

Thanks. Failing CI is now only due to known issues.

@tmadlener tmadlener merged commit a925cb5 into key4hep:main Apr 11, 2024
6 of 10 checks passed
@Zehvogel Zehvogel deleted the event-display branch April 11, 2024 11:21
jmcarcell added a commit that referenced this pull request Apr 23, 2024
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