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

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

This pull request closes theAxes.stackplot task in issue#26859

Plot output

PR checklist

ResolvesAxes.stackplot in#26864
[N/A] new and changed code istested
[N/A]Plotting related features are demonstrated in anexample
[N/A]New Features andAPI Changes are noted with adirective and release note
[N/A] Documentation complies withgeneral anddocstring guidelines

add test_stackplot in test_datetime.py

d96219a

github-actionsbot reviewed

Oct 17, 2023

View reviewed changes

Copy link

github-actionsbot 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 uson gitter for real-time discussion.

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

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

ksunden requested changes

Oct 17, 2023

View reviewed changes

Copy link

Member

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 thatstackplot doesn't quite work as intended forunitful 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:

matplotlib/lib/matplotlib/stackplot.py

Line 81 in978b402

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

Thepromote_types call there actually works for datetimes passed asobject arrays (which is why this test works at all), butwill fail fornp.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 thatpromote_types call could be problematic...

lib/matplotlib/tests/test_datetime.py Outdated

		[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

ksundenOct 17, 2023

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 areactually avoiding the most likely problematic year by starting at 1, but it is still close enough to throw some alarm bells off)

lib/matplotlib/tests/test_datetime.py Outdated

Comment on lines 350 to 354

		)
		ax2.set_ylim(
		datetime.datetime(2022, 12, 31),
		datetime.datetime(2023, 2, 8)
		)

Copy link

Member

ksundenOct 17, 2023

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 ofstackplot.

I would support extending the API of thebaseline 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 rathernot test the "timedelta on the y axis" case than provide this test which is not a sensible use ofdatetime (though I think it is notfar off, and you made something that lookedokay... I just think that the tools are not there to make using datetimes sensible here)

I do thinktimedelta 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)

Remove stackplot test with datetime y-axis

3cf5376

Copy link

ContributorAuthor

QuadroTec commentedOct 17, 2023•
edited
Loading

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?

Copy link

Member

ksunden commentedOct 17, 2023

I think the inability to set a baseline was the only problem fordatetime.datetime object arrays, but there is a lurking problem fornp.datetime64 arrays, which are usually interoperable withdatetime.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.

ksunden approved these changes

Oct 17, 2023

View reviewed changes

Copy link

Member

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 ofstackplot is a good goal. (And once implemented, this test can be updated to include such)