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

'show_titles' should reflect the specified quantiles (?) #107

Closed
JamesSample opened this issue Nov 28, 2017 · 9 comments · Fixed by #193
Closed

'show_titles' should reflect the specified quantiles (?) #107

JamesSample opened this issue Nov 28, 2017 · 9 comments · Fixed by #193
Labels

Comments

@JamesSample
Copy link

Not sure whether this is a very minor issue or just me misunderstanding the intended behaviour...

If I specify quantiles in my call to corner.corner(), I get vertical lines on the histograms at my specified quantiles, but the plot titles always assume quantiles=[0.16, 0.5, 0.84] (see here).

Shouldn't show_titles use the specified quantiles? Perhaps something like this for lines 277 to 282:

if title_fmt is not None:
    # Compute the quantiles for the title. This might redo
    # unneeded computation but who cares.
    q_l, q_50, q_u = quantile(x, quantiles, weights=weights)
    q_m, q_p = q_50-q_l, q_u-q_50

Thanks for an awesome package!

@dfm dfm added the bug label Nov 28, 2017
@dfm
Copy link
Owner

dfm commented Nov 28, 2017

This is definitely a bug either with the docs or the code, but I think that it might be better to just fix the docstring to reflect the current behavior. Alternatively, there could be an error thrown if the user doesn't provide exactly 3 quantiles.

@JamesSample
Copy link
Author

Thanks Dan. I guess it is useful to be able to plot more than three quantile lines if desired, so perhaps updating the docstring is the best way to go, as you suggest. Intervals for the specified quantiles can still be obtained by passing verbose=True if desired.

@benjaminrose
Copy link

Is this the same issue as #94?

@alessandropeca
Copy link

Any news?

@benjaminrose
Copy link

benjaminrose commented Mar 27, 2020

@alessandropeca, I am still slowly working on #108, but it is not done yet. That is not exactly the same as this issue, but they are related. I can share the WIP code I have if you (or anyone) wishes.

@jhmatthews
Copy link
Contributor

I've just come across this issue. This seems important to fix since it'd be quite easy to quote incorrect 90% confidence intervals, for example, particularly for quite steep posterior distributions.

Unless #108 is nearly done @benjaminrose , I'd suggest a simpler fix where one additional kwarg is used (say, title_quantiles) to manually set the quantiles use in the calculation of the +- errorbars. If None (default), then use quantiles as the title_quantiles.

The old behaviour could be recovered by always passing title_quantiles=[0.16,0.5,0.84]. Note this wouldn't be as flexible/powerful as the proposed fix #108 but IMO sufficient.

@jhmatthews
Copy link
Contributor

The slight problem with the simple fix above is that quantiles can be any length, and title_quantiles must be lenth-3, so this must be checked. It also needs to deal with the scenario that ``len(quantiles) == 0, and title_quantiles is None```, probably by defaulting to the 1 sigma quantiles.

@benjaminrose
Copy link

@jhmatthews #108 was half implemented, but then fell behind the main repo. The work I have might be a pain to merge. I would fix what needs to be fixed and not worry about the half working code I wrote.

@jhmatthews
Copy link
Contributor

I've proposed a fairly simple fix for this under PR #193.

@dfm dfm linked a pull request Feb 22, 2022 that will close this issue
@dfm dfm closed this as completed in #193 Feb 22, 2022
cdrischler added a commit to cdrischler/nuclear_saturation that referenced this issue Feb 2, 2023
checks with a corner plot obtained by brute-force sampling.

The Python package `corner` has a bug, which causes the percentiles
in the figure titles to default to hard-coded values. The issue is known
dfm/corner.py#107.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants