Add argument confidence_interval_root_find indicating root finding method#265
Add argument confidence_interval_root_find indicating root finding method#265
confidence_interval_root_find indicating root finding method#265Conversation
Pull Request Test Coverage Report for Build 22446669536Details
💛 - Coveralls |
|
On a first quick look this looks very nice! Could you think of a quick unites to make sure it stays that nice? 😄 |
|
Thanks @hammannr . I have added a simple test of the new method |
hammannr
left a comment
There was a problem hiding this comment.
I didn't run it yet, but just had a look at the code. Looks mostly good, and I will try running it asap. In the meantime, I left a few comments on things I noticed so far.
| nominal_values={"sigma": 1.0}, | ||
| parameter_definition=gaussian_model_parameter_definition, | ||
| compute_confidence_interval=COMPUTE_CONFIDENCE_INTERVAL, | ||
| confidence_interval_root_find="extremal", |
There was a problem hiding this comment.
Not really tests the method, but only if it fails to load it, no?
There was a problem hiding this comment.
sure but this doesnt' test if the function works properly, does it?
| xL, | ||
| xR, | ||
| which="left", | ||
| step=0.01, |
There was a problem hiding this comment.
Maybe we should assert that step is >=0, otherwise this will give unexpected behaviour or infinity loops.
| xR, | ||
| which="left", | ||
| step=0.01, | ||
| step_growth=1.0, |
There was a problem hiding this comment.
This option generally seems a bit unstable to me since it can explode or converge to zero, no?
There was a problem hiding this comment.
Yes, it might explode, so I set the default step_growth as 1.0, and the step does not increase.
| confidence_interval_root_find (str, optional (default=None)): | ||
| root finding algorithm of confidence interval |
There was a problem hiding this comment.
For convenience, also state here the two options, please.
alea/utils.py
Outdated
| """Find the left-most or right-most root of f in [xL, xR] using adaptive scanning + brentq | ||
| refinement.""" |
There was a problem hiding this comment.
Please add a brief explanation for all the parameters of the function.
|
I played with it a little bit and it' quite easy to find a case in which this produces def function_with_six_roots(x):
return (x - 1) * (x - 2) * (x - 3) * (x - 4) * (x - 5)* (x - 6)
left_extremal = extremal_root(
function_with_six_roots,
xL=-100,
xR=100,
which="left")though this will fail without readjusting the step size: def function_with_six_small_roots(x):
return (x - 1e-5) * (x - 2e-5) * (x - 3e-5) * (x - 4e-5) * (x - 5e-5)* (x - 6e-5)
left_extremal = extremal_root(
function_with_six_small_roots,
xL=-100,
xR=100,
which="left",)Do you think there is a sensible way to make this a bit more robust? |
Maybe 1% of the parameter's boundary? |
|
I hope the analysts will figure out how to scale the parameter of interest to be within meaningful values. I will print a warning when the step is too large/small compared to the poi's boundary. |
When calculating CL, if
confidence_interval_root_findisbrentq, usescipy.optimize.brentq; ifextremal, look for the left or right-most root.The left- right- most root finding is performed in
utils.extremal_root. This function accepts almost the same arguments asscipy.optimize.brentq.