TopEFT / topeft

15 stars 24 forks source link

Changes on topeft side to use the updated coffea JME tool from topcoffea repo #430

Closed anpicci closed 1 month ago

anpicci commented 2 months ago

Linked to https://github.com/TopEFT/topcoffea/pull/43

It provides the necessary tweaks to use the coffea JME script from topcoffea instead of using it from coffea package.

bryates commented 2 months ago

@anpicci please update the CI to checkout the clib_JME branch of TopCoffea for now.

bryates commented 2 months ago

@anpicci please update the CI to checkout the clib_JME branch of TopCoffea for now.

And please fix the Lint errors on this branch too https://github.com/TopEFT/topeft/actions/runs/10885709080/job/30203965586?pr=430#step:4:32

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 26.58%. Comparing base (3d3ad9d) to head (e948c3a). Report is 16 commits behind head on master.

Files with missing lines Patch % Lines
analysis/topeft_run2/analysis_processor.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #430 +/- ## ======================================= Coverage 26.58% 26.58% ======================================= Files 31 31 Lines 4612 4612 ======================================= Hits 1226 1226 Misses 3386 3386 ``` | [Flag](https://app.codecov.io/gh/TopEFT/topeft/pull/430/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/430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TopEFT) | `26.58% <0.00%> (ø)` | | 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.

anpicci commented 1 month ago

Hi @bryates , in terms of CI checks, this is the output:

--- Sample ---

ttH
    2lss_p
    24.440747910357743
    2lss_m
    23.682660030475528
    2lss_4t_p
    3.7420859206878716
    2lss_4t_m
    3.143570254916079
    3l_p_offZ_1b
    7.092358176813738
    3l_m_offZ_1b
    6.427546341470231
    3l_p_offZ_2b
    6.380945449879321
    3l_m_offZ_2b
    5.333479134731798
    3l_onZ_1b
    2.8117177784153666
    3l_onZ_2b
    2.7504305251895733
    4l
    0.6938357941053654
    2lss_CRflip
    2.7070414721666367
    2lss_CR
    19.146009554634293
    3l_CR
    4.772572300291932
    2los_CRtt
    2.538816259025149
    2los_CRZ
    3.601390236350366

--- Percent diff between New yields and Ref yields ---

ttH
    2lss_p
    1.5628481951278415e-10
    2lss_m
    -2.745451671921262e-10
    2lss_4t_p
    -5.266152868186154e-10
    2lss_4t_m
    2.6016745508012555e-10
    3l_p_offZ_1b
    -3.6833035360561243e-10
    3l_m_offZ_1b
    -1.3619521950188777e-10
    3l_p_offZ_2b
    -1.3710788700782723e-10
    3l_m_offZ_2b
    1.175950496879962e-10
    3l_onZ_1b
    -6.478572562008918e-10
    3l_onZ_2b
    -5.884360529627142e-10
    4l
    -2.547342357061768e-10
    2lss_CRflip
    -4.03292608718438e-10
    2lss_CR
    -1.1415605959170803e-10
    3l_CR
    2.4835437282842654e-10
    2los_CRtt
    -1.3090857233686402e-10
    2los_CRZ
    -1.9165547088399032e-09

In the attached csv, you can find the comparison at the fit level. Wilson_Coefficients_Comparison_Updated.csv

bryates commented 1 month ago

Hi @bryates , in terms of CI checks, this is the output:

--- Sample ---

ttH
    2lss_p
  24.440747910357743
    2lss_m
  23.682660030475528
    2lss_4t_p
  3.7420859206878716
    2lss_4t_m
  3.143570254916079
    3l_p_offZ_1b
  7.092358176813738
    3l_m_offZ_1b
  6.427546341470231
    3l_p_offZ_2b
  6.380945449879321
    3l_m_offZ_2b
  5.333479134731798
    3l_onZ_1b
  2.8117177784153666
    3l_onZ_2b
  2.7504305251895733
    4l
  0.6938357941053654
    2lss_CRflip
  2.7070414721666367
    2lss_CR
  19.146009554634293
    3l_CR
  4.772572300291932
    2los_CRtt
  2.538816259025149
    2los_CRZ
  3.601390236350366

--- Percent diff between New yields and Ref yields ---

ttH
    2lss_p
  1.5628481951278415e-10
    2lss_m
  -2.745451671921262e-10
    2lss_4t_p
  -5.266152868186154e-10
    2lss_4t_m
  2.6016745508012555e-10
    3l_p_offZ_1b
  -3.6833035360561243e-10
    3l_m_offZ_1b
  -1.3619521950188777e-10
    3l_p_offZ_2b
  -1.3710788700782723e-10
    3l_m_offZ_2b
  1.175950496879962e-10
    3l_onZ_1b
  -6.478572562008918e-10
    3l_onZ_2b
  -5.884360529627142e-10
    4l
  -2.547342357061768e-10
    2lss_CRflip
  -4.03292608718438e-10
    2lss_CR
  -1.1415605959170803e-10
    3l_CR
  2.4835437282842654e-10
    2los_CRtt
  -1.3090857233686402e-10
    2los_CRZ
  -1.9165547088399032e-09

In the attached csv, you can find the comparison at the fit level. Wilson_Coefficients_Comparison_Updated.csv

Thanks for the thorough tests @anpicci! Once all the above comments are done, I'll be happy with merging this PR. @Andrew42 and @sscruz do you have any other comments?

anpicci commented 1 month ago

@bryates great! I think I have addressed all the comments, but feel free to point me to any pending comment I haven't caught yet

bryates commented 1 month ago

Outdated

Thanks! @Andrew42 and @sscruz do you have any final comments on this PR?

anpicci commented 1 month ago

Now it should be removed @bryates

anpicci commented 1 month ago

@Andrew42 I have committed some changes to address your comments

Andrew42 commented 1 month ago

@anpicci the changes look fine to me. The only thing I might ask is to add a docstring or some comment to the ApplyTES and ApplyFES functions to give some context or description about what they're doing.

anpicci commented 1 month ago

@Andrew42 , regarding ApplyTES and ApplyFES, since they are going to be changed by John's implementation in https://github.com/TopEFT/topeft/pull/431/, should we move the docstring enhancement there, in order to avoid double effort?

Andrew42 commented 1 month ago

sure