Skip to content

Add back getattr for ExtensionArrays #10278

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

Merged
merged 8 commits into from
May 8, 2025

Conversation

dcherian
Copy link
Contributor

@dcherian dcherian commented May 1, 2025

@ilan-gold
Copy link
Contributor

  1. (fix): remove _getattr__ method for PandasExtensionArray #10250 (comment) should be considered here i.e., using __getattribute__ like I had had before the removal
  2. @martinfleis PandasExtensionArray was also removed from the public API, but getattr was just a pass-through to the underlying array, which is now used in e.g., Variable.data instead of the PandasExtensionArray wrapper. So I would caution being 100% the two are linked.

@martinfleis
Copy link
Contributor

I did not have time to pinpoint a specific commit but it has to be one of the commits merged in last 7 days, where this one felt like the only sensible option. But I might be wrong.

@ilan-gold
Copy link
Contributor

@martinfleis I will have a look then! @dcherian What is xarray's breaking change policy? I was under the impression that there was none given the date-versioning, but should there be stronger guarantees?

@dcherian
Copy link
Contributor Author

dcherian commented May 1, 2025

We try hard to not break things. Though in. this case, we're still figuring out how to handle extension arrays.

Seems like there are two options:

  1. add back the passthrough
  2. xvec extracts the underlying array.

I lean toward one, since that makes sense for a wrapper. I don't see any way other than __getattribute__ sadly, i thought __getattr__ would just work.

@ilan-gold
Copy link
Contributor

@dcherian My caution/suspicion was warranted. I just checked out xvec and xarray before the removal of getattr. The test is still broken. What I did notice is that the dtype of df[c] on the breaking line is preserved now, which is more in line with the breaking change in: #9671. However, this is probably an upgrade for you @martinfleis - the xarray object now preserves the geometry data type because it was a coordinate and therefore under-the-hood, wrapped in an indexing adapter which #9671 addressed. So the conversion is likely no longer necessary.

@ilan-gold
Copy link
Contributor

@dcherian As for bringing back this feature, I am neutral on it because it is not in the public API. I liked your suggestion of removing it because it means that things are explicit now. But of course, if you want to bring it back, __getattribute__ is a must.

@martinfleis I will open a PR

@ilan-gold
Copy link
Contributor

@dcherian It would be great if we could merge this. We have lost the nbytes implementation so I think it might be only a matter of time until someone complains:

import geopandas, geodatasets

geopandas.read_file(geodatasets.get_path('geoda.malaria')).to_xarray()

dcherian added 3 commits May 7, 2025 12:17
* main:
  dev whats-new (pydata#10294)
  Add SeasonGrouper, SeasonResampler (pydata#9524)
  (fix): remove `PandasExtensionArray` from repr (pydata#10291)
  Do not rely on `np.broadcast_to` to perform trivial dimension insertion (pydata#10277)
  DOC: Remove reference to absolufy (pydata#10290)
  Fix for scalar detection (pydata#8821)
  Add Index.validate_dataarray_coord (pydata#10137)
  add redirect for contributing guide (pydata#10282)
  Update pre-commit hooks (pydata#10288)
  Adding xarray-eopf to ecosystem.rst (pydata#10289)
  Add public typing.py module (pydata#10215)
  Rename Twitter to X (pydata#10283)
  Add `xarray-lmfit` extension for curve fitting to ecosystem documentation (pydata#10262)
@dcherian dcherian requested a review from ilan-gold May 7, 2025 18:23
@dcherian dcherian added plan to merge Final call for comments and removed plan to merge Final call for comments labels May 7, 2025
Co-authored-by: Ilan Gold <ilanbassgold@gmail.com>
@dcherian dcherian enabled auto-merge (squash) May 8, 2025 15:04
auto-merge was automatically disabled May 8, 2025 15:08

Pull Request is not mergeable

@dcherian dcherian closed this May 8, 2025
@dcherian dcherian reopened this May 8, 2025
@dcherian dcherian enabled auto-merge (squash) May 8, 2025 15:30
@dcherian dcherian merged commit 7d00e0a into pydata:main May 8, 2025
61 checks passed
@dcherian dcherian deleted the fix-getattr-extension branch May 8, 2025 16:28
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.

4 participants