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

do not force_ndarray_like #234

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

Conversation

burnpanck
Copy link

@TomNicholas
Copy link
Member

TomNicholas commented Dec 14, 2023

Thanks for opening this @burnpanck .

I notice in tests/test_accessors.py we also separately set force_ndarray_like=True

unit_registry = UnitRegistry(force_ndarray=True)

That seems odd in that it doesn't fully decouple the tests from the code, and also seems like it might be making this PR misleading by causing tests to pass in the CI that would not work in user code?

@burnpanck
Copy link
Author

Didn't spot that one. However, the tests still pass on my machine. To me, this suggests that force_ndarray_like is really not needed for any of the core xarray/pint interop to work. The only case that has been offered is the automatic coercion of a scalar quantity into a zero-dimensional array in the construction of a DataArray. But I strongly suggest that we add a test for that if we even want to claim that this is an actual feature.

@keewis
Copy link
Collaborator

keewis commented Dec 15, 2023

As I pointed out in #216 (comment), it is possible to use pint + xarray without pint-xarray, so most of the tests that do check compatibility live in xarray's test suite (for now, there's the desire to move those tests here once we have a replacement that still does extensive checks of the duck array compatibility).

After changing the unit registry of these tests to have force_ndarray_like=False 107 tests are failing, so clearly this is important for pint support in xarray.

@burnpanck
Copy link
Author

it is possible to use pint + xarray without pint-xarray

I'll have to process that. I'm wondering why one should be using pint-xarray in the first place then. I'll try to have a go at reproducing those failing tests in xarray so I understand better.

@keewis
Copy link
Collaborator

keewis commented Dec 15, 2023

pint-xarray is a glue / convenience package: while it is possible to use (most of) pint + xarray without pint-xarray, it is not very convenient to do so: creating quantities, converting between them, and serializing back to array + attribute would be very complex otherwise (however, this will change with #163, which will allow having units on indexed coordinates, something that currently is not possible).

@burnpanck
Copy link
Author

burnpanck commented Nov 11, 2024

I would like to pick this up again. In our company, we have internal libraries that depend on pint-pandas, and libraries that depend on pint-xarray. As mentioned elsewhere, those two are fundamentally incompatible because of pint-xarray forcing global changes into the environment. In the past, we worked around that by pushing a build from this branch to an internal registry and using that, but that is a deployment complication that I would like to avoid.

Because of our experience with that and the precedent of pint-pandas, I'm still not convinced that pint-xarray cannot be fixed/working around the issues in pint without force_ndarray_like=True. So, to pick this up, I should now prove that, by showing that all tests can be made to pass. I realise that with xarray-array-testing, the landscape for testing things may have changed in the meantime. @keewis could you give me short pointer on how pint + xarray tests are currently being run?

@keewis
Copy link
Collaborator

keewis commented Nov 16, 2024

on how pint + xarray tests are currently being run?

xarray-array-testing does not cover everything yet, so you'd still have to run the tests in xarray/tests/test_units.py to see which parts of xarray will fail (once it does cover everything, though, the xarray(pint(**)) tests will be added and run in this repository)

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.

avoid setting force_ndarray_like = True
3 participants