-
Notifications
You must be signed in to change notification settings - Fork 23
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: Add support for POWHEG weights #220
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #220 +/- ##
==========================================
+ Coverage 88.18% 91.48% +3.30%
==========================================
Files 2 2
Lines 220 235 +15
Branches 48 54 +6
==========================================
+ Hits 194 215 +21
+ Misses 23 16 -7
- Partials 3 4 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Hello @APN-Pucky, Thank you for this addition 👍. I think it would be very important to add your test file to scikit-hep-testdata and have it used in the test suite here, to enhance what is covered. Can you kindly take care of that? Agree and any comments, @matthewfeickert? |
So just PR a simple LHE file to https://github.com/scikit-hep/scikit-hep-testdata and wait for a release then included that here? |
Precisely :) I will not be much available until Wednesday but you can ping Jim or Matthew for example. Advance thanks. |
FYI I've now made a release with your files - https://github.com/scikit-hep/scikit-hep-testdata/releases/tag/v0.4.36. I hope it will show up in PyPI soon. I'm not sure if @matthewfeickert is still around for a review. I will not be working much more until the new year :-). Enjoy the Christmas break and "talk to you" in 2024! |
Thank you for the effort of getting this through in this year. It is by no means in a hurry, so lets wait for next year. Merry Christmas and happy new year to everyone reading this. |
Thanks for adding me to the particle contributors. What's the status of this MR? |
Hi. Seems @matthewfeickert and/or @lukasheinrich as experts have been overlooking this. Let's see now with a reping ... As for your question above, it sounds good to me. Is there anything else you intend to change in this PR or else it is totally ready from your side? |
This PR is done from my side since I already changed the access to the weighs to be by the id name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the nice additions. I'm approving and will be merging tomorrow unless we hear from @matthewfeickert on other feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @APN-Pucky! This is great, and we should cut a new relase after this. I just have a few questions and requested changes, but this is looking good overall!
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Co-authored-by: Eduardo Rodrigues <[email protected]>
Co-authored-by: Eduardo Rodrigues <[email protected]>
Co-authored-by: Matthew Feickert <[email protected]>
a7d52b6
to
c25df6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you @APN-Pucky!
@all-contributors please add @APN-Pucky for code. |
I've put up a pull request to add @APN-Pucky! 🎉 |
@APN-Pucky Release |
powheg uses plain text weights in lhe, like:
pwgevents-0001.lhe.zip