[hist] remove default parameter in TH1::GetQuantiles, as is the case with TF1::GetQuantiles #16793
Conversation
…with TF1::GetQuantiles Fixes root-project#16784 The currently default parameter of p = nullptr is a very weird use case, which calculates F-1(F(bin_edges)), ie it just returns the bin edges. So force user to really decide if he wants to pass that nullptr as argument.
Test Results 17 files 17 suites 3d 11h 52m 27s ⏱️ For more details on these failures, see this check. Results for commit 900abcf. |
There was a problem hiding this comment.
Hi,
I am not sure if somebody is using that functionality. I agree is weird, but it is not returning the bin edge, because in case of empty bins at the begining (and end) of the axis, it returns the first (last) non-empty bin edges. It could be useful in some cases.
In case we drop, we should probably remove alltogether the functionality.
| @@ -4552,7 +4552,7 @@ TVirtualHistPainter *TH1::GetPainter(Option_t *option) | |||
| /// | |||
| /// \param[in] n maximum size of array xp and size of array p (if given) | |||
| /// \param[out] xp array to be filled with nq quantiles evaluated at (p). Memory has to be preallocated by caller. | |||
| /// If p is null (default value), then xp is actually set to the (first n) histogram bin edges | |||
There was a problem hiding this comment.
The comment is maybe not correct.
xp is always set to nbin+1 value, even if n is less than nbin+1
There was a problem hiding this comment.
This simple tests shows the opposite: #16782 (comment)
xp is filled with the first 20 bin edges, even if there are 100 bins.
Fixes #16784
The currently default parameter of p = nullptr is a very weird use case, which calculates F-1(F(bin_edges)), ie it just returns the bin edges. So force user to really decide if he wants to pass that nullptr as argument.
Warning: this PR might break some preexisting scripts though relying on this weird feature.