Skip to content

Adding Scalar as a Baseline Option #27308

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

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

EricRLeao1311
Copy link

PR summary

Allowing an int to be received as a baseline option in stackplots, to prevent cases where the baseline is not sensible enough.
Closes #27129

PR checklist

@ksunden
Copy link
Member

ksunden commented Nov 13, 2023

I think that restricting to an int is deeply flawed, really we want any scalar value (including unitful scalars, such as datetime objects). Certainly at least there is no reason to disallow floating point scalar values if we are allowing setting the baseline to arbitrary values.

I also think that perhaps even more important than the code change allowing baseline to be scalar is the documentation (and testing) aspects of this code change, which are not reflected here.

@pedrompecanha
Copy link
Contributor

pedrompecanha commented Nov 19, 2023

I'm pair programming alongside @EricRLeao1311. We've changed to code so it accepts floats, but we still have some questions about testing and documentation. About testing, how could we improve/fix it? We found some tests in test_axes.py, but they use a @image_comparison wrapper, and we are unsure on how to make it work for our case. We tried to include some cases, but we don't know if they were made correctly. And about documentation, we attempted to improve the docstrings related to the function. Is that enough? Thanks!

try:
axs[0, 0].stackplot(range(100), d.T, baseline=5)
except ValueError as e:
print("Error when using an integer as baseline:", e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are testing something that should fail the test if the error is raised, then remove the try/except.

If you are testing that this should raise, then use with pytest.raises(...) which will ensure that the exception is actually raised (where printing will just ignore whether or not an exception was raised)

@ksunden
Copy link
Member

ksunden commented Nov 21, 2023

I still think float is too restrictive, and that we should be looking at unit-aware changes, so while float is a lot better than specifically int, I still don't think it is far enough

@pedrompecanha
Copy link
Contributor

Ok, I see that you want it to be more expansive than float-only, but we are having trouble on what types we should accept and how to treat the different types. Could you enlighten us in any type of way?

@ksunden
Copy link
Member

ksunden commented Nov 28, 2023

Because the units system can have arbitrary values, there is no restriction on the type, much like the bottom value for e.g. ax.bar, which is documented as "float or arraylike", but also contains an explicit note about if it has units.

https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.bar.html

This should be treated similarly, in that it gets passed through axes._process_unit_info:

# It is possible for y (bottom) to contain unit information.

Where this has odd behavior:

  • categorical data (in general), as you cannot just add them in their raw forms and so I don't think it can make sense here (you could maybe add them post-conversion, but I think you actually want to convert most things after adding...)
  • categorical data (in specific), even if we sorted out the general problems, if you happen to have a category called "zero", "sym", "wiggle", etc, then you would be unable to specify those as the baseline... This is an edge case I don't see as too big of a problem, but it is odd.

I think that it could be:

  • remove the check_in_list entirely
  • add an else case to the if baseline == chain which calls axes._process_unit_info(("y", baseline)) sets firstline = baseline and does stack += baseline (there may be some shape massaging for 1D data, like there is for sym, but for scalar data it should just be +=
  • Do something about the np.promote_types call... not sure exactly what, but e.g. datetime64 should not be promoted (and will error)

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

Successfully merging this pull request may close these issues.

[ENH]: Better support for stackplot with units [particularly datetimes]
3 participants