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

Pandas 2.2.0 test fixes #8638

Merged
merged 17 commits into from
Jan 22, 2024
Merged

Conversation

nameloCmaS
Copy link
Contributor

@nameloCmaS nameloCmaS commented Jan 21, 2024

Fixes tests mentioned in Pull request #8633 comments.

Further errors in tests etc still remain.

@nameloCmaS nameloCmaS changed the title Pandas 2.2.0 doc fix Pandas 2.2.0 test fixes Jan 21, 2024
@max-sixty
Copy link
Collaborator

@nameloCmaS thank you v much!

@spencerkclark any thoughts? You know this better obv. I'll merge if no objections shortly, and we can make any further adjustments along with #8629

doc/whats-new.rst Outdated Show resolved Hide resolved
Copy link
Member

@spencerkclark spencerkclark left a comment

Choose a reason for hiding this comment

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

Indeed this is awesome, thanks! I think there is maybe one more doctest failure if you don't mind:

=================================== FAILURES ===================================
______________ [doctest] xarray.core.dataarray.DataArray.curvefit ______________
6342                  0.04744543,  0.03602333,  0.03129354,  0.01074885,  0.01284436,
6343                  0.00910995]])
6344         Coordinates:
6345           * x        (x) int64 0 1 2
6346           * time     (time) int64 0 1 2 3 4 5 6 7 8 9 10
6347 
6348         Fit the exponential decay function to the data along the ``time`` dimension:
6349 
6350         >>> fit_result = da.curvefit("time", exp_decay)
6351         >>> fit_result["curvefit_coefficients"].sel(
Differences (unified diff with -expected +actual):
    @@ -1,4 +1,4 @@
     <xarray.DataArray 'curvefit_coefficients' (x: 3)>
    -array([1.05692035, 1.73549638, 2.9421577 ])
    +array([1.05692036, 1.73549638, 2.94215771])
     Coordinates:
       * x        (x) int64 0 1 2

/home/runner/work/xarray/xarray/xarray/core/dataarray.py:6351: DocTestFailure
=========================== short test summary info ============================
FAILED xarray/core/dataarray.py::xarray.core.dataarray.DataArray.curvefit
============= 1 failed, 299 passed, 2 skipped in 65.48s (0:01:05) ==============

xarray/tests/test_accessor_dt.py Outdated Show resolved Hide resolved
xarray/tests/test_cftime_offsets.py Outdated Show resolved Hide resolved
@max-sixty
Copy link
Collaborator

Indeed this is awesome, thanks! I think there is maybe one more doctest failure if you don't mind:

Yeah, I've had this before. I think aarch64 has different precision, which is annoying. Reverting will make CI pass...

Changed to code-block rather than suppressing the warning as PR comment.
Reverted due to different precision on aarch64 leading to different output.
@max-sixty max-sixty added the plan to merge Final call for comments label Jan 22, 2024
@max-sixty
Copy link
Collaborator

@nameloCmaS we can merge as soon as the conflict is resolved...

@max-sixty
Copy link
Collaborator

@nameloCmaS we can merge as soon as the conflict is resolved...

I tried resolving myself, not 100% sure if correctly; will merge if tests pass

Copy link
Collaborator

@mathause mathause left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. As a heads up: #8642 might lead to more docstring changes. (Depending on the merge order I can also do the updates in #8642.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file is generated - do we need to update https://github.com/pydata/xarray/blob/main/xarray/util/generate_aggregations.py instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, great spot @mathause ...

@max-sixty
Copy link
Collaborator

For clarity (+because would be great to merge this; it fixes the doctests), a couple of items remaining:

@nameloCmaS let us know if you want a hand!

@nameloCmaS
Copy link
Contributor Author

For clarity (+because would be great to merge this; it fixes the doctests), a couple of items remaining:

@nameloCmaS let us know if you want a hand!

Thank you; I'll have a go at both points now. I'll let you know if I get stuck.

Reversed changes
Fixed hangover from clashing commits.
Updated with xarray/util/generate_aggregations.py
@nameloCmaS
Copy link
Contributor Author

Ok... I have redone the changes to _aggregations.py but this time officially as follows:

conda activate xarray-tests
pip install pytest-accept
python xarray/util/generate_aggregations.py
pytest --doctest-modules xarray/core/_aggregations.py --accept || true
pytest --doctest-modules xarray/core/_aggregations.py

Discarded the namedarray/_aggregations.py file as the changes didn't look correct.

Corrected the correction...

ds = xray.Dataset({"t": pd.date_range("2000-01-01", periods=12, freq="M")})
ds["t.season"]
In[92]: ds = xray.Dataset({"t": pd.date_range("2000-01-01", periods=12, freq="M")})
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this switch to ME? (We'll see in the docs build, so can wait if you're not sure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "code-block" part and then the ":: ipython" above the code means it is static but with syntax highlighting. Doc.

I did this as I didn't want to "change history" and my quick fix of :okwarning: was not ok. The code was correct at v0.4 so shouldn't be refactored to run on modern code.

@max-sixty
Copy link
Collaborator

Great @nameloCmaS !

If you want to add a whatsnew now or in another PR, please feel free, this deserve some recognition :)

@nameloCmaS
Copy link
Contributor Author

Great @nameloCmaS !

If you want to add a whatsnew now or in another PR, please feel free, this deserve some recognition :)

Thank you! I made a few errors along the way with a lot of help. I will do it in another PR so this one can hopefully be merged.

@max-sixty max-sixty merged commit d6deb46 into pydata:main Jan 22, 2024
29 checks passed
@nameloCmaS nameloCmaS deleted the pandas-2.2.0-doc-fix branch January 23, 2024 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plan to merge Final call for comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants