UxDataset and UxDataArray should not allow uxgrid=None

[Sevans711]

Version

v2026.7.0

How did you install UXarray?

Source

What happened?

UxDataset and UxDataArray currently allow uxgrid=None, but their docstrings say "If None, it needs to be an instance of uxarray.Grid." I'm not really sure what that is supposed to mean.

Additionally, using uxgrid=None leads to confusing errors later, such as AttributeError: 'NoneType' object has no attribute 'n_face'. It would be much easier to debug an error like TypeError: UxDataArray.__init__() missing one required argument: 'uxgrid' instead.

What did you expect to happen?

I expected instantiating UxDataset and UxDataArray to fail if uxgrid is not provided.

Or, I expected a docstring which correctly clarifies what happens if uxgrid is None, perhaps providing at least one example use-case clarifying why uxgrid=None is supported?

Can you provide a MCVE to repoduce the bug?

[erogluorhan]

What instantiation method are you referring to?

[Sevans711]

What instantiation method are you referring to?

Construction directly from the class constructor. I now realize this would have been clearer if I provided an MVE originally. Apologies about that! See below:

For example:

import uxarray as ux
arr0 = ux.UxDataArray([1,2,3])

I would expect this to fail, since no uxgrid is provided, however it succeeds without issue.

Confusing AttributeErrors will occur when performing operations if the input array looks more like reasonable uxarray data, such as by having the n_face dimension:

import uxarray as ux
arr1 = ux.UxDataArray(xr.DataArray([1,2,3], dims=['n_face']))
arr1.plot()

This makes: AttributeError: 'NoneType' object has no attribute 'n_face'. That is confusing; my initial thought would be "arr1 certainly has n_face, so why am I getting an AttributeError?" (The actual reason is: I forgot to supply a uxgrid).

[erogluorhan]

Ah, that's helpful; thank you!

It doesn't make sense to allow None for uxgrid in these constructors, and I checked MOST of the occurrences in the code, and they provide a Grid object already (as expected).

[Sevans711]

Ah, upon actually trying to implement the change, I am beginning to understand why None was accepted before. Many things, e.g. isel() end up calling _replace(); the UxDataArray implementation is to basically just call super()._replace() and then set result.uxgrid = self.uxgrid. Sadly, xarray.DataArray._replace() ends with return type(self)(variable, coords, name=name, indexes=indexes, fastpath=True) (without any option to pass through some arbitrary **kwargs). In our case, type(self) is UxDataArray at this point. So, requiring None during UxDataArray.__init__ is incompatible with this method.

Here are two possible ways to proceed:

• (1) Continue to allow uxgrid=None during UxDataArray.__int__ and UxDataset.__init__. But, update docstring to clarify it must be set to something before any computations are performed or things will crash. And, add a comment internally saying the reason for this (some abbreviated version of the description above). Additionally, change the uxgrid property to include a clearer error message if uxgrid has not been set yet. Something like:

    @property
    def uxgrid(self):
        if self._uxgrid is None:
            raise GridInvalidError("failed to provide a valid value for self.uxgrid! Expected a Grid object, got None")
        # else, do what the code currently does on `main` branch right now:
        return self._uxgrid

• (2) Push for actually disallowing uxgrid=None. Doing that in the near future would require a somewhat hacky solution.
  • (2a) The simplest short-term solution would be copy-pasting xarray's internal code for _replace and editing it to allow passing through the uxgrid kwarg, instead of using super()._replace(). This solution is brittle if xarray updates those methods some day. There are also similar methods which need to be copy-pasted for the UxDataset case.
  • In the long-term, we could push for xarray to make subclassing easier by allowing kwargs to be passed through to __init__ in internal methods like this, or to use something like self._new(...) instead of type(self)(...), with self._new(...) defaulting to type(self)(...) but overidable by subclasses. (For example, we would want self._new(*args, **kwargs) to return type(self)(*args, **kwargs, uxgrid=self.uxgrid))
  • (2b) A more complicated short-term solution would be to dynamically construct a new class on the fly whose type(self)(...) loosely wraps UxDataArray's, but also knows to use uxgrid=self.uxgrid. I'm pretty confident it's possible, and would probably be more elegant than (2a) in that it would not actually be brittle, but it might require constructing something quite "clever"/"amusing", like class _LocallyWrappedUxDataArray(), whose definition depends on self.uxgrid, inside of the _replace method itself…. Basically, we would be hacking our way around the fact that xarray doesn't allow kwargs to pass through to type(self)(...) internally. But, it would be a permanent solution, it wouldn't break later if xarray updated it's _replace method later, unlike (2a).

My intuition/guess is that we will go with option (1), because it gets us 90% of the way there; there wouldn't be any more confusing errors or docstrings. But, maybe we will want something in (2) if we really want to fully remove the None kwarg? I would be pretty comfortable setting up something like (2b), and it shouldn't actually take that long to do, but I would want a green flag from someone else before trying it. @erogluorhan please let me know how you would prefer to proceed, whenever you get a chance to take another look at this!

There are also a lot of internal details in this message… if you want any clarifications please don't hesitate to ask, or maybe it would be faster to set up a meeting to talk through it out loud?

[erogluorhan]

Thanks for looking into this, @Sevans711 ! Let's put low effort and go the route (1) then (route 2 might not be worth the effort as constructors are not commonly used, and they seem to be used already with valid grids anyway)! To make sure:

Keep uxgrid=None allowed — just improve the docstring & comments explaining why, and make the uxgrid property raise a clear GridInvalidError if accessed while still None.