Remove default value of p from TH1::GetQuantiles() as is the case with TF1::GetQuantiles

[AntoniMarcinek]

Explain what you would like to see improved and how.

This is a follow-up to #16736, see https://github.com/root-project/root/pull/16782#discussion_r1820807153

Depending on whether `p` argument is `nullptr` or not, the method actually does 2 different things. It either calculates `xp` given `p` or it calculates `p`. So it is either F^-1(p) or F(x).

The default parameter of p = nullptr brings the method to return just the bin edges, so it is not very useful.

ROOT version

master, although notation matches what #16782 brings

Installation method

irrelevant

Operating system

irrelevant

Additional context

No response

[ferdymercury]

The wording is not strictly correct:

• If p is not null, it calculates F-1(p)
• If p is null, it calculates F-1(F(bin_edges)) = bin_edges

[ferdymercury]

Maybe someone is using this second variant (p=null) for something, so changing this would break backward compatibility.

What is exactly your use case / what you want to achieve?

If what you want is to have a new method to get F(x), then that method already exists, it is TH1::GetCumulative, so not sure if it's useful to add a new function name for this.

Or what bothers you is that it is a default argument?

[AntoniMarcinek]

Oh. Missed GetCumulative. The thing that probably bothers me is the description of GetQuantiles or rather what is the use case of GetQuantiles with p==nullptr. When you say "If p is null, it calculates F-1(F(bin_edges)) = bin_edges", the bin_edges are already known (if it really calculates them inside, that is quite surprising). The really new thing (although maybe not a product, but side-effect) is the p.

I understand that removing the default would break backward compatibility, so I am not advocating just removing the thing in the next release. It's just that if there is a default argument, normally it represents a frequent use case, so it brings attention while learning about a given method. Here it brings confusion (to me and to my student at least).

[AntoniMarcinek]

Given what you wrote and what I wrote, I no longer have a firm opinion on what should be done with it unfortunately...

[ferdymercury]

he really new thing (although maybe not a product, but side-effect) is the p.

But p stays null. F(x) is stored to a temporary variable, p is not set to F(x).

(if it really calculates them inside, that is quite surprising)

It does. It's a technical workaround to have both paths to follow the same code.

It's just that if there is a default argument, normally it represents a frequent use case, so it brings attention while learning about a given method. Here it brings confusion (to me and to my student at least).

I agree with you.

So I would suggest to remove from your title "add new method to handle this case". Making it non-default would force everyone to pass an argument. If one wants the old behavior, it will pass nullptr and get the bin edges as before, so no new extra method is needed.

[ferdymercury]

Side note, the default value for p is there since 23y: https://github.com/root-project/root/commit/95d7d30c95900387abaf7b39bf014df5ff5e4a32

[AntoniMarcinek]

I missed that p stays null. You showed it in the comment to the other MR...

In this case I don't understand what was the idea 23 years ago. I can't imagine the use case for this.

I think the current title for the issue is good.

[dpiparo]

We need @lmoneta for clarifying this...