TopEFT / topeft

15 stars 24 forks source link

datacard making update #406

Closed ywan2 closed 4 months ago

ywan2 commented 6 months ago

Two features are added in the datacard making step using python make_cards.py pkl _np.pkl.gz:

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 85.71429% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 27.44%. Comparing base (8095829) to head (e96fedb).

:exclamation: Current head e96fedb differs from pull request most recent head 36b68d4

Please upload reports for the commit 36b68d4 to get more accurate results.

Files Patch % Lines
topeft/modules/datacard_tools.py 80.95% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #406 +/- ## ========================================== + Coverage 26.27% 27.44% +1.16% ========================================== Files 31 28 -3 Lines 4552 4245 -307 ========================================== - Hits 1196 1165 -31 + Misses 3356 3080 -276 ``` | [Flag](https://app.codecov.io/gh/TopEFT/topeft/pull/406/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TopEFT) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/TopEFT/topeft/pull/406/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TopEFT) | `27.44% <85.71%> (+1.16%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TopEFT#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ywan2 commented 6 months ago

Thank you @bryates for your comments! I made the changes you suggested, except for the last one of "checking non-zero nominal for zero FFUp / FFDown ". There are some channels that have these values during this step but don't raise errors in Combine. Also, I think that keeping "print()" statement might be better than "raise Warning", in case people want to still have the rest of the datacards made to test some things. What do you think?

bryates commented 6 months ago

Thank you @bryates for your comments! I made the changes you suggested, except for the last one of "checking non-zero nominal for zero FFUp / FFDown ". There are some channels that have these values during this step but don't raise errors in Combine. Also, I think that keeping "print()" statement might be better than "raise Warning", in case people want to still have the rest of the datacards made to test some things. What do you think?

@ywan2 Raising a warning should still just print a message and continue. Raising an exception will for sure terminate the script. Are you seeing otherwise?

bryates commented 6 months ago

Thank you @bryates for your comments! I made the changes you suggested, except for the last one of "checking non-zero nominal for zero FFUp / FFDown ". There are some channels that have these values during this step but don't raise errors in Combine. Also, I think that keeping "print()" statement might be better than "raise Warning", in case people want to still have the rest of the datacards made to test some things. What do you think?

@ywan2 Raising a warning should still just print a message and continue. Raising an exception will for sure terminate the script. Are you seeing otherwise?

Ah maybe I was thinking of this (https://stackoverflow.com/questions/3891804/raise-warning-in-python-without-interrupting-program). You can switch back to print statements instead of warnings.

ywan2 commented 6 months ago

@bryates, yes I tested the the raise Warning statement, it does terminate the code. So I switched back to print statement.

bryates commented 4 months ago

Merging this PR now. I've created issues for the remaining items so we can address them later.