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

feat: adding python based full chain example for the ODD #1230

Merged
merged 12 commits into from
Apr 28, 2022

Conversation

asalzburger
Copy link
Contributor

This PR adds a python based full chain example for the Open Data Detector, very along the lines of the ITk example.

@asalzburger asalzburger added this to the next milestone Apr 11, 2022
@codecov
Copy link

codecov bot commented Apr 11, 2022

Codecov Report

Merging #1230 (3ee3d96) into main (996267c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1230   +/-   ##
=======================================
  Coverage   47.94%   47.94%           
=======================================
  Files         373      373           
  Lines       19495    19495           
  Branches     9152     9152           
=======================================
  Hits         9347     9347           
  Misses       3817     3817           
  Partials     6331     6331           

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@asalzburger
Copy link
Contributor Author

I need to change the |eta| range +/- 4 does not make sense

@paulgessinger
Copy link
Member

I have some small updates. Do you mind me pushing to your branch directly? For that I believe you'd have to enable commits from maintainers.

@timadye
Copy link
Contributor

timadye commented Apr 12, 2022

If we are adding option parsing now, should we also do it for full_chain_itk.py, or as a separate PR?

Other options to consider:

  • set logLevel
  • Allow Pythia8 or evgen input files (eg. options for HS process, pileup, and input filename)

@paulgessinger
Copy link
Member

Personally I would restrict it to be the barest possible minimum. I can remove it again if you feel like this should be consistent across the "full chain" examples.

@timadye
Copy link
Contributor

timadye commented Apr 14, 2022

I had another idea. Could we have a common.py function to define Sequence and/or evgen inputs (particle gun, Pythia8, or file input) based on command-line options. That could be used in full_chain_itk.py and full_chain_odd.py without adding much extra code to distract from the example they give. It would still be "slide-ware" 😄

If you agree, I can try to put something like that together. Maybe we don't need the options for this PR, but they are useful as a first step for a separate change.

@paulgessinger
Copy link
Member

I removed the command line arguments and did some cleanup.

Copy link
Contributor

@timadye timadye left a comment

Choose a reason for hiding this comment

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

looks good :)

@kodiakhq kodiakhq bot merged commit fc2ee7f into acts-project:main Apr 28, 2022
@paulgessinger paulgessinger modified the milestones: next, v19.0.0 May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants