Closed biemster closed 1 year ago
Merging #200 (36a7108) into main (277a30b) will decrease coverage by
0.16%
. The diff coverage is54.54%
.
@@ Coverage Diff @@
## main #200 +/- ##
==========================================
- Coverage 97.32% 97.16% -0.16%
==========================================
Files 33 33
Lines 2986 2994 +8
==========================================
+ Hits 2906 2909 +3
- Misses 80 85 +5
Files Changed | Coverage Δ | |
---|---|---|
puma/roc.py | 91.50% <54.54%> (-2.25%) |
:arrow_down: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more
Thanks @biemster, this looks good. I think you are missing a docstring description for the new argument added to add_roc()
.
I also wondered whether you would be happy to show the two taggers on separate ratio panels? If this would suit, we could perhaps consider generalising the existing ratio groups and reference=True
logic to support custom ratio groups, (instead of automatically assigning ratio groups based on rejection class), rather than adding the key
, reference_key
logic as you have done here.
So instead of
plot_roc.add_roc(
Roc(
sig_eff,
fastGN1_ujets_rej_r29,
n_test=n_jets_light_r29,
rej_class="ujets",
signal_class="bjets",
label="fastGN1 r29",
linestyle="dashed",
),
key="fastGN1_r29",
)
plot_roc.add_roc(
Roc(
sig_eff,
fastGN1_ujets_rej_r30,
n_test=n_jets_light_r30,
rej_class="ujets",
signal_class="bjets",
label="fastGN1 r30",
linestyle="solid",
),
key="fastGN1_r30",
reference_key="fastGN1_r29",
)
we could have something like
`plot_roc.add_roc(
Roc(
sig_eff,
fastGN1_ujets_rej_r29,
n_test=n_jets_light_r29,
rej_class="ujets",
signal_class="bjets",
label="fastGN1 r29",
linestyle="dashed",
),
ratio_group="fastGN1",
reference=True,
)
plot_roc.add_roc(
Roc(
sig_eff,
fastGN1_ujets_rej_r30,
n_test=n_jets_light_r30,
rej_class="ujets",
signal_class="bjets",
label="fastGN1 r30",
linestyle="solid",
),
ratio_group="fastGN1",
)
If the ratio_group
argument is not specified, we would fall back to assigning it based on the the rej_class
of the ROC.
This implementation would bit more consistent with the way the ratio groups are implemented currently, rather than having two sets of logic on how to group ROCs. It might also lead to cleaner ratio panels with fewer curves on them since groups would be split into different panels (though you might not actually want that!)
Let me know what you think. Cheers!
Hi @samvanstroud , I added the docstring. For validating our taggers between releases we prefer just a single ratio panel actually. But if I just think about this for a second I can probably come up with a couple use cases where separate panels for different taggers suits better, so it's probably a good idea to open a separate ticket for this and collect ideas?
Thanks @biemster! Ok, since this is already useful to you I'm happy to merge it and we can discuss further in https://github.com/umami-hep/puma/issues/202 if needed :) thanks for the change
This change adds support to set a per curve reference to the ROC curves, besides a reference per reject class. This allows to make ROC curve comparisons between datasets, for example between two validation rounds:
This adds a parameter
reference_key
toadd_roc()
, so something like the following can be done: