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

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

Merged
merged 3 commits into from
Dec 6, 2023

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Dec 3, 2023

Currently xcorr and acorr are in test_datetimes.py. However, I think it is highly unlikely anyone would want to pass datetimes in as x 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.

@story645
Copy link
Member

story645 commented Dec 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 these https://online.stat.psu.edu/stat462/node/188/

@jklymak
Copy link
Member Author

jklymak commented Dec 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.

@story645
Copy link
Member

story645 commented Dec 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 to https://matplotlib.org/devdocs/users/explain/axes/axes_units.html#more-about-unit-support or https://matplotlib.org/devdocs/api/units_api.html

Copy link
Contributor

@oscargus 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 Outdated Show resolved Hide resolved
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@jklymak jklymak force-pushed the mnt-remove-xcorr-acorr branch 2 times, most recently from 07ed125 to 1f37e49 Compare December 4, 2023 23:39
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/axes/_axes.py Outdated Show resolved Hide resolved
lib/matplotlib/tests/test_datetime.py Outdated Show resolved Hide resolved
@QuLogic QuLogic merged commit e13216e into matplotlib:main Dec 6, 2023
29 of 30 checks passed
@QuLogic QuLogic added this to the v3.9.0 milestone Dec 6, 2023
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

5 participants