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

Having title_quantiles=None when quantiles is passed without having exactly 3 elements raises a ValueError #237

Open
sybreton opened this issue Sep 19, 2023 · 4 comments · May be fixed by #244

Comments

@sybreton
Copy link

Hi,

The behaviour of the title_quantiles argument that was introduced in v2.2.2 is responsible for a crash when using the following set of arguments:
show_titles=True, title_quantiles=None
and a list for quantiles that has a number of elements other than 3. In this case, according to what is written in the documentation, the value for title_quantiles is set as what is passed for quantiles, and a ValueError is raised.

In particular, this is an issue for compatibility with codes that were running with corner versions older than v2.2.2.

The error can be reproduced by slightly modifying the code from the Getting started tutorial (https://corner.readthedocs.io/en/latest/pages/quickstart/):

import corner
import numpy as np

ndim, nsamples = 2, 10000
np.random.seed(42)
samples = np.random.randn(ndim * nsamples).reshape([nsamples, ndim])
figure = corner.corner(samples, quantiles=[0.16, 0.84], 
                       show_titles=True, title_quantiles=None)

Running this code, the following error is raised:
ValueError: 'title_quantiles' must contain exactly three values; pass a length-3 list or array using the 'title_quantiles' argument

@sahiljhawar
Copy link

As a regular corner user this error makes complete sense to me. Can you tell why this behaviour is not desired?

@sybreton
Copy link
Author

sybreton commented Nov 30, 2023

Well I have not specified it explicitly in the issue description above, but the problem is that quantiles is not an argument that requires mandatorily 3 elements as title_quantiles, here is what the documentation says:

quantiles (iterable) – A list of fractional quantiles to show on the 1-D histograms as vertical dashed lines.

Thus passing a list with arbitrary length should not raise any exception (and it also means that upgrading to corner==2.2.2 breaks codes that were working fine with previous versions).

To clarify even more, here is the expected behaviour if you run the example code above with corner=2.2.1:

import corner
import numpy as np
print (corner.__version__)

ndim, nsamples = 2, 10000
np.random.seed(42)
samples = np.random.randn(ndim * nsamples).reshape([nsamples, ndim])
figure = corner.corner(samples, quantiles=[0.16, 0.84], 
                       show_titles=True, title_quantiles=None)

2.2.1

output

@sahiljhawar
Copy link

sahiljhawar commented Nov 30, 2023

I really dont see any problem with this. title_quantiles and quantiles are different yet same. And I guess the code changes in 2.2.2 is useful. title_quantiles of course needs 3 values, else how will you show the mean and std? Morever if your quantiles is already a 3-vector which is used to draw the dashed lines then just passing show_titles=True is more than sufficient. In case you only pass 2-vector to quantiles, the the title_quantiles needs to passed which should be a 3-vector. One thing that can be done is to default to 1-sigma values if the title_quantiles is not passed (which internally is None)

@sybreton
Copy link
Author

sybreton commented Nov 30, 2023

The main problem is backward compatibility: any code that ran smoothly before v2.2.2 now breaks, even if the quantiles value it provides is still valid from what can be read in the documentation.
From what I see this is an actual issue due to the former behaviour of show_titles and quantiles which did not actually reflect what was written in the documentation (from what I understand and see from older versions of the code, while quantiles was supposed to impact what was shown in the title, the quantiles were actually hardwired to [0.16, 0.5, 0.84]). You can take a look at issue #193 which mentions the corresponding issue.

Anyway, I submitted as a suggestion pull request #244, hopefully it restores backward compatibility while maintaining the expected behaviour of title_quantiles when provided (and I took care of clarifying in the docstrings the role of each argument, the description of show_titles seemed obsolete).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants