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

Revert .oindex and .vindex additions in _ElementwiseFunctionArray, NativeEndiannessArray, and BoolTypeArray classes #8921

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

andersy005
Copy link
Member

As noted in #8909, the use of .oindex and .vindex properties in coding/* appears to have broken some backends (e.g. scipy). This PR reverts those changes. We plan to bundle these changes into a separate backends feature branch (see this comment, which will be merged once we are confident about its impact on downstream dependencies.

…`, `NativeEndiannessArray`, and `BoolTypeArray` classes
@andersy005 andersy005 requested a review from dcherian April 8, 2024 17:11
@andersy005
Copy link
Member Author

@ocraft, when you get a chance do you mind try this branch and confirming whether this fixes your issue in #8909? as @dcherian noted, the minimal verifiable example you provided in #8909 (comment) wasn't reproducible on his end.

@andersy005 andersy005 force-pushed the revert-indexing-changes-to-coding branch from aacbe04 to 721aaf1 Compare April 8, 2024 18:15
@ocraft
Copy link

ocraft commented Apr 9, 2024

Ok, I started from scratch and created venv with this set of packages (newest version of xarray and scipy with deps):

Package         Version
--------------- -----------
numpy           1.26.4
packaging       24.0
pandas          2.2.1
pip             22.0.4
python-dateutil 2.9.0.post0
pytz            2024.1
scipy           1.13.0
setuptools      58.1.0
six             1.16.0
tzdata          2024.1
xarray          2024.3.0

and my example results in error as in #8909 (comment)
After uninstalling xarray and installing it from this pull request with

pip install https://github.com/pydata/xarray/pull/8921/commits/721aaf1f4dc88d8801717246941d9e63379c9aef

I got:

NotImplementedError: _ElementwiseFunctionArray._oindex_get method should be overridden

@andersy005
Copy link
Member Author

@dcherian /@ocraft, i believe i've managed to revert the breaking changes. however, the temporary fixes turned out to be messier than i expected :(

Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

yuck but ok :)

@andersy005
Copy link
Member Author

@ocraft, when you get a chance can you confirm this fully addresses the issue on your end?

@ocraft
Copy link

ocraft commented Apr 11, 2024

@ocraft, when you get a chance can you confirm this fully addresses the issue on your end?

I have this error: NotImplementedError: StackedBytesArray._oindex_get method should be overridde

@dcherian
Copy link
Contributor

Can we add the reproducer as a test case please

@dcherian
Copy link
Contributor

Can we add the reproducer as a test case please

@andersy005 can we add a test here please?

@andersy005
Copy link
Member Author

Can we add the reproducer as a test case please

@andersy005 can we add a test here please?

i wanted to mention that, as i revisit this issue to add a test, i'm learning the temporary fix may become more complex than initially thought. should we attempt to fix this going forward (as part of the adding support for oindex/vindex to BackendArray) rather than reverting to the previous behavior?

@andersy005
Copy link
Member Author

i can confirm that this issue is fully resolved on backend-indexing branch

In [1]: import xarray as xr
   ...: 
   ...: ds = xr.Dataset()
   ...: ds['A'] = xr.DataArray([[1, 'a'], [2,'b']],dims=['x','y'])
   ...: ds.to_netcdf('/tmp/test.nc')
   ...: ds2 = xr.open_dataset('/tmp/test.nc')
   ...: ds2.sel(y=[1]).to_netcdf('/tmp/test.nc')


In [2]: 

In [2]: xr.show_versions()

INSTALLED VERSIONS
------------------
commit: e96e70e04dc21607173d7cdcd9dba2c121d6aab6
python: 3.12.3 | packaged by conda-forge | (main, Apr 15 2024, 18:35:20) [Clang 16.0.6 ]
python-bits: 64
OS: Darwin
OS-release: 23.4.0
machine: arm64
processor: arm
byteorder: little
LC_ALL: None
LANG: en_US.UTF-8
LOCALE: ('en_US', 'UTF-8')
libhdf5: None
libnetcdf: None

xarray: 2024.3.1.dev39+g04cca97a0
pandas: 2.2.2
numpy: 1.26.4
scipy: 1.13.0
netCDF4: None
pydap: None
h5netcdf: None
h5py: None
zarr: None
cftime: None
nc_time_axis: None
iris: None
bottleneck: None
dask: None
distributed: None
matplotlib: None
cartopy: None
seaborn: None
numbagg: None
fsspec: None
cupy: None
pint: None
sparse: None
flox: None
numpy_groupies: None
setuptools: 69.5.1
pip: 24.0
conda: None
pytest: None
mypy: None
IPython: 8.22.2
sphinx: None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ScipyArrayWrapper' object has no attribute 'oindex'
3 participants