Use ndf instead of probability for vertexes in EDM4hep #71
Conversation
k4EDM4hep2LcioConv/include/k4EDM4hep2LcioConv/k4Lcio2EDM4hepConv.ipp
Outdated
Show resolved
Hide resolved
| @@ -174,6 +176,29 @@ std::vector<CollNamePair> convertReconstructedParticles(const std::string& name, | |||
| return results; | |||
| } | |||
|
|
|||
| // Go from chi^2 and probability (1 - CDF(chi^2, ndf)) | |||
| // to ndf by a binary search | |||
| int find_ndf(double chi2, double prob) { | |||
There was a problem hiding this comment.
Since this is not a template, we could put this into the header (and potentially into a nested namespace) and move the implementation to the cc file.
There was a problem hiding this comment.
Done, I think a namespace for a single small function is a bit overkill, no?
| // Initial guess for the upper bound. If it's not enough, it will be increased | ||
| int upper = 100; | ||
| while (TMath::Prob(chi2, upper) < prob) { | ||
| lower = upper; |
|
Another consideration is that if the probability is > 1 or chi^2 < 0 then it will be an infinite loop. I don't know if those cases have ever happened, but if they do then someone will have to debug why it's not doing anything... |
|
Maybe we can add an |
|
I added a throw since it's going to be very annoying to run some chain of algorithms that never ends and having to figure out why that is happening, better throw before. |
There was a problem hiding this comment.
CI is failing because key4hep/EDM4hep#324 is not yet merged, but that cannot be merged without breaking the nightlies, without this. I assume that you have tested this locally, so I think this can go in, together with the EDM4hep PR.
tmadlener
force-pushed
the
probability-ndf
branch
from
0aba1cc to
d7fe460
Compare
Changes after key4hep/EDM4hep#324. I'm finding ndf by a binary search in the LCIO to EDM4hep direction and seems to work fine. Maybe the upper value can be tuned with typical values to reduce a few steps in the search.
BEGINRELEASENOTES
ENDRELEASENOTES