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

add test_stackplot in test_datetime.py #27114

Merged
merged 3 commits into from
Oct 18, 2023

Conversation

QuadroTec
Copy link
Contributor

@QuadroTec QuadroTec commented Oct 17, 2023

PR summary

Adds the test_stackplot method in test_datetime.py. The test creates the following stackplots:

  1. datetime x-axis, stacked numerical y-axis
  2. datetime x-axis, datetime plus stacked datetime.timedelta y-axis (not implemented for this pull request)

Stacking multiple datetime arrays caused a compile error, because numpy does not support adding two datetime objects together. I don't believe it makes much sense to stack datetime values in this way, so I have not implemented this test.

This pull request closes the Axes.stackplot task in issue #26859

Plot output
test_stackplot output

PR checklist

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

Copy link
Member

@ksunden ksunden left a comment

Choose a reason for hiding this comment

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

I think this has also uncovered that stackplot doesn't quite work as intended for unitful data (which is good, that is why we want these tests, to be clear)

I'll try to write up an issue later about it, but the thing that is throwing alarm bells to me is this line:

stack = np.cumsum(y, axis=0, dtype=np.promote_types(y.dtype, np.float32))

The promote_types call there actually works for datetimes passed as object arrays (which is why this test works at all), but will fail for np.datetime64 arrays (which we should either make work and add to this test or be clear about it not working....) I think that datetimes are a bit of an edge case here, but the broader point about units and that promote_types call could be problematic...

[datetime.datetime(2023, 1, i) for i in range(1, N)]
)
dates_years = np.array(
[datetime.datetime(1970 + i, 1, 1) for i in range(1, N)]
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid this date, as 1970 can hide problems where we don't actually convert dates properly.

(You are actually avoiding the most likely problematic year by starting at 1, but it is still close enough to throw some alarm bells off)

Comment on lines 350 to 354
)
ax2.set_ylim(
datetime.datetime(2022, 12, 31),
datetime.datetime(2023, 2, 8)
)
Copy link
Member

Choose a reason for hiding this comment

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

These limits are hiding the fact that the blue section goes all the way to 1970 because of the implicit 0 of stackplot.

I would support extending the API of the baseline kwarg to accept a scalar value, but that is not how it is implemented now, and I think that is out of the scope of this PR.

As it stands, I think I would rather not test the "timedelta on the y axis" case than provide this test which is not a sensible use of datetime (though I think it is not far off, and you made something that looked okay... I just think that the tools are not there to make using datetimes sensible here)

I do think timedelta is the right unit for this plot, to be clear... just it needs that extra explicit baseline, which is not currently supported (only zero or the various "wiggle" type options are supported currently)

@QuadroTec
Copy link
Contributor Author

QuadroTec commented Oct 17, 2023

Thanks for the review. I changed the starting year in the first test to 2020 from 1971. I also removed the second test until the baseline issue is addressed.

Just double-checking for the second test -- was the only problem the inability to set an explicit baseline, or were there other issues with how it was written?

@ksunden
Copy link
Member

ksunden commented Oct 17, 2023

I think the inability to set a baseline was the only problem for datetime.datetime object arrays, but there is a lurking problem for np.datetime64 arrays, which are usually interoperable with datetime.datetime. The test itself was fine, other than the baseline being an implicit 0 mark that does not make sense for the data (and having no way of setting a baseline)

I think that there could be some debate on whether the y axis of this plot should even be a concrete date (rather it is always an offset, perhaps), but we don't actually have handling for that built in yet...

Again, those shortcomings are of the current implementation, which while being not quite fully broken, is lacking polish, not of the test you wrote.

Copy link
Member

@ksunden ksunden left a comment

Choose a reason for hiding this comment

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

I think this test works for the functionality as it currently stands, though better support for the y axis of stackplot is a good goal. (And once implemented, this test can be updated to include such)

lib/matplotlib/tests/test_datetime.py Outdated Show resolved Hide resolved
lib/matplotlib/tests/test_datetime.py Outdated Show resolved Hide resolved
@QuadroTec
Copy link
Contributor Author

Hi @QuLogic, I have made the changes you suggested.

@QuLogic QuLogic added this to the v3.9.0 milestone Oct 18, 2023
@QuLogic QuLogic merged commit 1d1171f into matplotlib:main Oct 18, 2023
42 checks passed
@QuLogic
Copy link
Member

QuLogic commented Oct 18, 2023

Thanks @QuadroTec! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

@QuadroTec
Copy link
Contributor Author

Thank you @QuLogic!

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

Successfully merging this pull request may close these issues.

None yet

3 participants