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: Python particle gun and Fatras examples can be chained #1128

Merged
merged 8 commits into from
Feb 8, 2022

Conversation

timadye
Copy link
Contributor

@timadye timadye commented Dec 23, 2021

First step implementing #1098, incorporating #1093.

  • addParticleGun and addFatras adds the required modules to the Sequence.
  • runParticleGun and runFatras rewritten to use add* to reproduce exactly the previous behaviour.
  • ROOT_HASH_CHECKS=on pytest run as before without change.
    • Note that ROOT_HASH_CHECKS=on with pytest doesn't currently work from main. I have a couple of fixes for future PRs.
  • Optionally, runParticleGun and runFatras (like add*) can now accept outputDir as pathlib.Path as well as str. This should be the only change to how these routines work.

What do people think? If this is the right way to go, I can do the other Python examples - either in this PR or as a separate one if this one is merged.

asalzburger and others added 3 commits December 23, 2021 15:09
* `addParticleGun` and `addFatras` adds the required modules to the `Sequence`.
* `runParticleGun` and `runFatras` rewritten to use `add*` to reproduce exactly the previous behaviour.
* `ROOT_HASH_CHECKS=on pytest` run as before without change.
* Optionally `runParticleGun` and `runFatras` can now accept `outputDir` as `pathlib.Path` or `str` (like `add*`)
* Other Python examples not yet changed. Let's see how this one goes down.
@timadye timadye added the 🚧 WIP Work-in-progress label Dec 23, 2021
@timadye timadye added this to the next milestone Dec 23, 2021
@timadye timadye changed the title feat: Python particle_gun and fatras examples can be chained feat: Python particle gun and Fatras examples can be chained Dec 23, 2021
@codecov
Copy link

codecov bot commented Dec 23, 2021

Codecov Report

Merging #1128 (6e255a1) into main (853f9b7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1128   +/-   ##
=======================================
  Coverage   47.90%   47.90%           
=======================================
  Files         359      359           
  Lines       18491    18491           
  Branches     8723     8723           
=======================================
  Hits         8859     8859           
  Misses       3603     3603           
  Partials     6029     6029           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 853f9b7...6e255a1. Read the comment docs.

@timadye timadye removed the 🚧 WIP Work-in-progress label Jan 10, 2022
@timadye
Copy link
Contributor Author

timadye commented Jan 10, 2022

I have removed the WiP label. Maybe the WiP label has inhibited people from looking. This proposal has been complete (as far as it goes) and ready for review. I have not been working on it, but used the label originally because we could expand on it later.

Copy link
Member

@paulgessinger paulgessinger 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 I like this. I added some discussion points, but overall this feels like the right direction to me.

Should we add some form of example how to do the chaining in this PR already? Maybe in the docs?

Examples/Scripts/Python/particle_gun.py Outdated Show resolved Hide resolved
Examples/Scripts/Python/particle_gun.py Outdated Show resolved Hide resolved
Examples/Scripts/Python/particle_gun.py Show resolved Hide resolved
Examples/Scripts/Python/particle_gun.py Outdated Show resolved Hide resolved
@timadye
Copy link
Contributor Author

timadye commented Jan 11, 2022

Should we add some form of example how to do the chaining in this PR already? Maybe in the docs?

There is an example in runFatras. I'd prefer to hold off making docs until the other code is modified, so we can refer to more examples.

@paulgessinger
Copy link
Member

Fair enough @timadye.

Examples/Scripts/Python/particle_gun.py Outdated Show resolved Hide resolved
Examples/Scripts/Python/particle_gun.py Outdated Show resolved Hide resolved
Examples/Scripts/Python/particle_gun.py Show resolved Hide resolved
Examples/Scripts/Python/particle_gun.py Outdated Show resolved Hide resolved
Examples/Scripts/Python/particle_gun.py Outdated Show resolved Hide resolved
@timadye
Copy link
Contributor Author

timadye commented Jan 11, 2022

arrrgggg, GitHub foiled me again with its crazy-ass UI. I had been making all these comments and replies (one even before the holidays), but they all remained "Pending". I didn't spot that until just now and couldn't see what to do about it until I found that I had to go to the "Changes" section and click the "Review" button. Why oh why oh why....

@paulgessinger
Copy link
Member

About namedtuple and defaults: yeah it's more verbose, that's true. I think it should be possible to construct a namedtuple without specifying the field names, and I think there's also a way to supply defaults. Another alternative in the same vein would be dataclasses, I guess.

@timadye timadye removed the 🚧 WIP Work-in-progress label Feb 4, 2022
Copy link
Contributor

@robertlangenberg robertlangenberg left a comment

Choose a reason for hiding this comment

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

Thanks Tim, the changes look good!

@robertlangenberg robertlangenberg added automerge Feature Development to integrate a new feature labels Feb 8, 2022
@robertlangenberg robertlangenberg enabled auto-merge (squash) February 8, 2022 17:18
@robertlangenberg robertlangenberg merged commit ece76d4 into acts-project:main Feb 8, 2022
@paulgessinger paulgessinger modified the milestones: next, v17.1.0 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Feature Development to integrate a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants