Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
AntoniMarcinek opened this issue Oct 29, 2024 · 8 comments · May be fixed by #16793
Open

Comments

@AntoniMarcinek
Copy link

AntoniMarcinek commented Oct 29, 2024

Explain what you would like to see improved and how.

This is a follow-up to #16736, see #16782 (comment)

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
Copy link
Contributor

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
Copy link
Contributor

ferdymercury commented Oct 29, 2024

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
Copy link
Author

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
Copy link
Author

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

@ferdymercury
Copy link
Contributor

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 ferdymercury changed the title Please remove default value of p from TH1::GetQuantiles() and add new method to handle this case Please remove default value of p from TH1::GetQuantiles() Oct 29, 2024
@ferdymercury
Copy link
Contributor

Side note, the default value for p is there since 23y:
95d7d30

@AntoniMarcinek
Copy link
Author

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.

@ferdymercury ferdymercury changed the title Please remove default value of p from TH1::GetQuantiles() Remove default value of p from TH1::GetQuantiles() as is the case with TF1::GetQuantiles Oct 29, 2024
@dpiparo
Copy link
Member

dpiparo commented Oct 30, 2024

We need @lmoneta for clarifying this...

ferdymercury added a commit to ferdymercury/root that referenced this issue Oct 31, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Issues
Status: No status
4 participants