TopEFT / topeft

15 stars 24 forks source link

Remove unused code #350

Closed kmohrman closed 1 year ago

kmohrman commented 1 year ago

This PR removes scripts (e.g. plotting scripts, processors, etc) and files (e.g. SF files) that are outdated and unused. Some of the READMEs (mainly the one in analysis/topEFT) have also been updated to remove the description of the removed scripts (the the explanations for the remaining scripts have been somewhat reorganized).

Addresses Issues #336 and #343.

kmohrman commented 1 year ago

The CI runs, so it should be ok, but I can go ahead and run a full test run of the processor to make sure it's ok. By the way, I opened this PR against the technical_improvements branch not against master, as I think it'd probably be good to avoid messing with the master branch too much till TOP-22-006 is out the door. So would an alternative approach be to put off those final full sized tests till we have a few more of the technical modifications in place?

I think it'd be good if most/all of the updates we make to address the technical improvements issues in the GitHub project are made to the technical_improvements branch for now. Then once TOP-22-006 is finalized we can merge in this branch (or if we finish with all of the technical improvements long before TOP-22-006 is finalized so we want to merge it into master, we will probably want to do a lot of very thorough validation of the entire workflow to make sure everything looks ok beforehand).

codecov[bot] commented 1 year ago

Codecov Report

Merging #350 (e9371bd) into technical_improvements (239dda7) will increase coverage by 4.01%. The diff coverage is 20.00%.

@@                    Coverage Diff                     @@
##           technical_improvements     #350      +/-   ##
==========================================================
+ Coverage                   30.28%   34.30%   +4.01%     
==========================================================
  Files                          47       40       -7     
  Lines                        7600     6734     -866     
==========================================================
+ Hits                         2302     2310       +8     
+ Misses                       5298     4424     -874     
Flag Coverage Δ
unittests 34.30% <20.00%> (+4.01%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../topEFT/make_1d_quad_plots_from_template_histos.py 0.00% <0.00%> (ø)
analysis/topEFT/make_cr_and_sr_plots.py 0.00% <0.00%> (ø)
analysis/topEFT/missing_parton.py 0.00% <0.00%> (ø)
analysis/topEFT/parse_datacard_templtes.py 0.00% <0.00%> (ø)
topcoffea/modules/HistEFT.py 78.26% <ø> (ø)
topcoffea/scripts/make_html.py 14.03% <ø> (ø)
analysis/topEFT/make_1d_quad_plots.py 62.79% <100.00%> (ø)

... and 3 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

kmohrman commented 1 year ago

@bryates the full sized run of the processor just finished without any issues. And I also made cards from it (which also ran fine) and as a quick sanity check just verified that the observed number of events looked ok (and it indeed matched the right number).

kmohrman commented 1 year ago

Ok, so since I think the run of the processor was your only comment (and that seemed to check out), and since you've approved the PR, and since we're not even merging into master right now, just into the technical_improvements branch, I'll go ahead and merge now.