NotificationsYou must be signed in to change notification settings
Fork7.9k
Star21.3k

MNT/TST: remove xcorr and acorr from test_datetime#27427

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

QuLogic merged 3 commits intomatplotlib:mainfromjklymak:mnt-remove-xcorr-acorr

Dec 6, 2023

Merged

MNT/TST: remove xcorr and acorr from test_datetime#27427

QuLogic merged 3 commits intomatplotlib:mainfromjklymak:mnt-remove-xcorr-acorr

Dec 6, 2023

Conversation

Copy link

Member

jklymak commentedDec 3, 2023

Currentlyxcorr andacorr are intest_datetimes.py. However, I think it is highly unlikely anyone would want to pass datetimes in asx values to either of these methods. It is also not clear what giving the unit conversion means for these arguments. Modified the docstrings to make this clear.

jklymak force-pushed themnt-remove-xcorr-acorr branch from6a16664 to99fab6aCompare

December 3, 2023 17:06

jklymak mentioned this pull request

Dec 3, 2023

[TST]: increase unit test coverage#26864

Open

64 tasks

Copy link

Member

story645 commentedDec 4, 2023

However, I think it is highly unlikely anyone would want to pass datetimes in as x values to either of these methods

If we claim to support it, i.e. this participates in units, then it should probably be tested. Also timedeltas are common for thesehttps://online.stat.psu.edu/stat462/node/188/

Copy link

MemberAuthor

jklymak commentedDec 4, 2023

Xcorr and acorr do not participate in units.

Both work by assuming x and y are spaced equally in time (or space). You can of course assign units to the lags, but we do not try to do this in xcorr/acorr.

Copy link

Member

story645 commentedDec 4, 2023

Xcorr and acorr do not participate in units.

It might be good to generate a table of what participates in units and what doesn't, possibly added either tohttps://matplotlib.org/devdocs/users/explain/axes/axes_units.html#more-about-unit-support orhttps://matplotlib.org/devdocs/api/units_api.html

oscargus reviewed

Dec 4, 2023

View reviewed changes

Copy link

Member

oscargus left a comment

Choose a reason for hiding this comment

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

I agree that these should be removed. Some formatting, although I think that this probably should not go on the same line. Maybe better to add it on the next line? Or as an admonition?

lib/matplotlib/axes/_axes.py OutdatedShow resolvedHide resolved

lib/matplotlib/axes/_axes.py Outdated

		@@ -2055,7 +2056,9 @@ def xcorr(self, x, y, normed=True, detrend=mlab.detrend_none,

		Parameters
		----------
		x, y : array-like of length n
		x, y : array-like of length n. Neither x nor y are run through