Fix #884, remove code repetition in ProcessNormalization and AsymPow #989
Conversation
…and AsymPow regarding logKappaForX
|
It seems that the files I edited had unnecessary white spaces. My text editor automatically removes them. Is this ok? |
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
Can you not just call logKappa here, given that all logKappaForX does is call logKappa?
| double ret = avg + alpha*halfdiff; | ||
| //assert(alpha >= -1 && alpha <= 1 && "Something is wrong in the interpolation"); | ||
| return ret; | ||
| return logKappa(x, logKhi, logKlo); |
There was a problem hiding this comment.
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.
| #define logKappa_h | ||
|
|
||
| template<typename T> | ||
| T logKappa(const T x, const double &kappaHigh, const double &kappaLow) { |
There was a problem hiding this comment.
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?
| #define logKappa_h | ||
|
|
||
| template<typename T> | ||
| T logKappa(const T x, const double &kappaHigh, const double &kappaLow) { |
There was a problem hiding this comment.
| 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.
|
|
||
| template<typename T> | ||
| T logKappa(const T x, const double &kappaHigh, const double &kappaLow) { | ||
| if (fabs(x) >= 0.5) return (x >= 0 ? kappaHigh : -kappaLow); |
There was a problem hiding this comment.
| 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.
|
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: Mabye @adewit, what do you think? |
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:
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?