Fix #884, remove code repetition in ProcessNormalization and AsymPow #989

pitkajuh · 2024-07-06T14:16:51Z

Dear All. This PR fixes #884. Unfortunately I could not figure out a proper place for new logKappa function, so I created a new file for it. If there is a better place for it, please do not hesitate to tell me.

The code compiles without any problems but some test fail after running pytest via conda in test/ directory:

========================================================================== test session starts ==========================================================================
platform linux -- Python 3.8.0, pytest-8.2.2, pluggy-1.5.0
rootdir: /home/pitkajuh/bin/HiggsAnalysis/CombinedLimit/test
plugins: anyio-4.4.0
collected 0 items / 8 errors

================================================================================ ERRORS =================================================================================
________________________________________________________________ ERROR collecting validation/test_AS.py _________________________________________________________________
validation/test_AS.py:13: in <module>
    datacardGlob("simple-counting/counting-B5p5-Obs[16]*.txt"),
validation/TestClasses.py:439: in datacardGlob
    base = os.environ["CMSSW_BASE"] + "/src/HiggsAnalysis/CombinedLimit/data/benchmarks/"
../../../../anaconda3/envs/combine/lib/python3.8/os.py:673: in __getitem__
    raise KeyError(key) from None
E   KeyError: 'CMSSW_BASE'
________________________________________________________________ ERROR collecting validation/test_BS.py _________________________________________________________________
validation/test_BS.py:13: in <module>
    datacardGlob("simple-counting/counting-B5p5-Obs[16]*.txt"),
validation/TestClasses.py:439: in datacardGlob
    base = os.environ["CMSSW_BASE"] + "/src/HiggsAnalysis/CombinedLimit/data/benchmarks/"
../../../../anaconda3/envs/combine/lib/python3.8/os.py:673: in __getitem__
    raise KeyError(key) from None
E   KeyError: 'CMSSW_BASE'
________________________________________________________________ ERROR collecting validation/test_BT.py _________________________________________________________________
validation/test_BT.py:15: in <module>
    datacardGlob("simple-counting/counting-B5p5-Obs[16]*.txt"),
validation/TestClasses.py:439: in datacardGlob
    base = os.environ["CMSSW_BASE"] + "/src/HiggsAnalysis/CombinedLimit/data/benchmarks/"
../../../../anaconda3/envs/combine/lib/python3.8/os.py:673: in __getitem__
    raise KeyError(key) from None
E   KeyError: 'CMSSW_BASE'
________________________________________________________________ ERROR collecting validation/test_FC.py _________________________________________________________________
validation/test_FC.py:13: in <module>
    datacardGlob("simple-counting/counting-B5p5-Obs[16]*-Syst30U.txt"),
validation/TestClasses.py:439: in datacardGlob
    base = os.environ["CMSSW_BASE"] + "/src/HiggsAnalysis/CombinedLimit/data/benchmarks/"
../../../../anaconda3/envs/combine/lib/python3.8/os.py:673: in __getitem__
    raise KeyError(key) from None
E   KeyError: 'CMSSW_BASE'
________________________________________________________________ ERROR collecting validation/test_HN.py _________________________________________________________________
validation/test_HN.py:261: in <module>
    datacardGlob("simple-counting/counting-B5p5-Obs[16]*-S*[Uy].txt"),
validation/TestClasses.py:439: in datacardGlob
    base = os.environ["CMSSW_BASE"] + "/src/HiggsAnalysis/CombinedLimit/data/benchmarks/"
../../../../anaconda3/envs/combine/lib/python3.8/os.py:673: in __getitem__
    raise KeyError(key) from None
E   KeyError: 'CMSSW_BASE'
_______________________________________________________________ ERROR collecting validation/test_MCMC.py ________________________________________________________________
validation/test_MCMC.py:13: in <module>
    datacardGlob("simple-counting/counting-B5p5-Obs6*.txt"),
validation/TestClasses.py:439: in datacardGlob
    base = os.environ["CMSSW_BASE"] + "/src/HiggsAnalysis/CombinedLimit/data/benchmarks/"
../../../../anaconda3/envs/combine/lib/python3.8/os.py:673: in __getitem__
    raise KeyError(key) from None
E   KeyError: 'CMSSW_BASE'
________________________________________________________________ ERROR collecting validation/test_PLC.py ________________________________________________________________
validation/test_PLC.py:13: in <module>
    datacardGlob("simple-counting/counting-B5p5-Obs[16]*.txt"),
validation/TestClasses.py:439: in datacardGlob
    base = os.environ["CMSSW_BASE"] + "/src/HiggsAnalysis/CombinedLimit/data/benchmarks/"
../../../../anaconda3/envs/combine/lib/python3.8/os.py:673: in __getitem__
    raise KeyError(key) from None
E   KeyError: 'CMSSW_BASE'
________________________________________________________________ ERROR collecting validation/test_htt.py ________________________________________________________________
validation/test_htt.py:17: in <module>
    datacardGlob("htt/125/htt_*_8TeV.txt"),
validation/TestClasses.py:439: in datacardGlob
    base = os.environ["CMSSW_BASE"] + "/src/HiggsAnalysis/CombinedLimit/data/benchmarks/"
../../../../anaconda3/envs/combine/lib/python3.8/os.py:673: in __getitem__
    raise KeyError(key) from None
E   KeyError: 'CMSSW_BASE'
======================================================================== short test summary info ========================================================================
ERROR validation/test_AS.py - KeyError: 'CMSSW_BASE'
ERROR validation/test_BS.py - KeyError: 'CMSSW_BASE'
ERROR validation/test_BT.py - KeyError: 'CMSSW_BASE'
ERROR validation/test_FC.py - KeyError: 'CMSSW_BASE'
ERROR validation/test_HN.py - KeyError: 'CMSSW_BASE'
ERROR validation/test_MCMC.py - KeyError: 'CMSSW_BASE'
ERROR validation/test_PLC.py - KeyError: 'CMSSW_BASE'
ERROR validation/test_htt.py - KeyError: 'CMSSW_BASE'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 8 errors during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
========================================================================== 8 errors in 24.11s ===========================================================================

I am not sure if these errors have anything to do with the edits I made. I ran the tests by executing pytest command in test/ directory. I don't know if this is the proper way to run the tests. I did not find any information how to run tests. Can anyone provide any insight on this?

…and AsymPow regarding logKappaForX

pitkajuh · 2024-07-06T14:20:34Z

It seems that the files I edited had unnecessary white spaces. My text editor automatically removes them. Is this ok?

adewit

Hi, thanks for the PR. I just had a quick look but it would be great if other developers could also comment, since I'm not sure if there was a plan to fix the open issues such as the one that this one addresses immediately, as we may be doing a bigger overhaul in the future.

However, I already wanted to comment since I think in general it would be much preferred if you could test the code even just on a few different datacards to check that it is doing what you want, rather than relying on the unit tests that we can do as part of the PR (while quite extensive, we know they don't cover everything). I think it wasn't done in this case, since unless I misread the code, there is a minus sign error somewhere, which I would expect to change the behaviour.

adewit · 2024-07-06T14:33:13Z

src/ProcessNormalization.cc

@@ -93,7 +94,7 @@ Double_t ProcessNormalization::evaluate() const {
            const RooAbsReal *theta = asymmThetaListVec_.at(i);
            const std::pair<double,double> logKappas = logAsymmKappa_.at(i);
            double x = theta->getVal();
-            logVal +=  x * logKappaForX(x, logKappas);
+            logVal +=  x * logKappaForX(x, logKappas.second, logKappas.first);


Can you not just call logKappa here, given that all logKappaForX does is call logKappa?

adewit · 2024-07-06T14:38:14Z

src/AsymPow.cc

-    double ret = avg + alpha*halfdiff;
-    //assert(alpha >= -1 && alpha <= 1 && "Something is wrong in the interpolation");
-    return ret;
+    return logKappa(x, logKhi, logKlo);


This changes the behaviour of the original code, if I'm not mistaken, since you have already defined logKlo as -log(kappaLow_), but in the logKappa function you still put a minus sign in front of the variable kappaLo which, thus already represents -log(kappaLow_) in your code.

adewit · 2024-07-06T14:39:46Z

interface/logKappa.h

+#define logKappa_h
+
+template<typename T>
+T logKappa(const T x, const double &kappaHigh, const double &kappaLow) {


Given that what you are passing here are log(kappaHigh) and log(kappaLow), I think these variable names could be updated to something that matches better what they represent?

guitargeek · 2024-07-08T21:33:12Z

interface/logKappa.h

+#define logKappa_h
+
+template<typename T>
+T logKappa(const T x, const double &kappaHigh, const double &kappaLow) {


Suggested change

T logKappa(const T x, const double &kappaHigh, const double &kappaLow) {

inline double logKappa(double x, double kappaHigh, double kappaLow) {

Why the template? RooFit always deals with double anyway. And why the references? Doubles are usually passed by value.

guitargeek · 2024-07-08T21:35:22Z

interface/logKappa.h

+
+template<typename T>
+T logKappa(const T x, const double &kappaHigh, const double &kappaLow) {
+    if (fabs(x) >= 0.5) return (x >= 0 ? kappaHigh : -kappaLow);


Suggested change

if (fabs(x) >= 0.5) return (x >= 0 ? kappaHigh : -kappaLow);

if (std::abs(x) >= 0.5) return (x >= 0 ? kappaHigh : -kappaLow);

Better to use functions from the C++ standard library and don't forget to #include <cmath> in this file to be sure to have the math functions.

guitargeek · 2024-07-08T21:38:39Z

Very nice initiative!

About the place to put your function: I would create a new file to collect these free math functions, just like we do in RooFit:
https://github.com/root-project/root/blob/master/roofit/roofitcore/inc/RooFit/Detail/MathFuncs.h#L86

Mabye CombineMathFuncs.h. Then we can also put other repeated code there.

@adewit, what do you think?

guitargeek · 2024-11-19T21:35:49Z

@pitkajuh, you can close this PR, and alternative PR has been merged. Thanks for the initial work!

Fix cms-analysis#884, remove code repetition in ProcessNormalization …

14e8707

…and AsymPow regarding logKappaForX

adewit reviewed Jul 6, 2024

View reviewed changes

guitargeek reviewed Jul 8, 2024

View reviewed changes

guitargeek mentioned this pull request Nov 18, 2024

Factor out some mathematical details into free functions #1017

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #884, remove code repetition in ProcessNormalization and AsymPow #989

Fix #884, remove code repetition in ProcessNormalization and AsymPow #989

pitkajuh commented Jul 6, 2024

pitkajuh commented Jul 6, 2024

adewit left a comment

adewit Jul 6, 2024

adewit Jul 6, 2024

adewit Jul 6, 2024

guitargeek Jul 8, 2024

guitargeek Jul 8, 2024

guitargeek commented Jul 8, 2024 •

edited

Loading

guitargeek commented Nov 19, 2024

	T logKappa(const T x, const double &kappaHigh, const double &kappaLow) {
	inline double logKappa(double x, double kappaHigh, double kappaLow) {

	if (fabs(x) >= 0.5) return (x >= 0 ? kappaHigh : -kappaLow);
	if (std::abs(x) >= 0.5) return (x >= 0 ? kappaHigh : -kappaLow);

Fix #884, remove code repetition in ProcessNormalization and AsymPow #989

Are you sure you want to change the base?

Fix #884, remove code repetition in ProcessNormalization and AsymPow #989

Conversation

pitkajuh commented Jul 6, 2024

pitkajuh commented Jul 6, 2024

adewit left a comment

Choose a reason for hiding this comment

adewit Jul 6, 2024

Choose a reason for hiding this comment

adewit Jul 6, 2024

Choose a reason for hiding this comment

adewit Jul 6, 2024

Choose a reason for hiding this comment

guitargeek Jul 8, 2024

Choose a reason for hiding this comment

guitargeek Jul 8, 2024

Choose a reason for hiding this comment

guitargeek commented Jul 8, 2024 • edited Loading

guitargeek commented Nov 19, 2024

guitargeek commented Jul 8, 2024 •

edited

Loading