xarray icon indicating copy to clipboard operation
xarray copied to clipboard

map_over_datasets: skip empty nodes

Open mathause opened this issue 1 year ago • 9 comments

  • [x] Closes #9693
  • [x] Closes #10013
  • [ ] Tests added
  • [ ] User visible changes (including notable bug fixes) are documented in whats-new.rst
  • [ ] New functions/methods are listed in api.rst

  • misses tests and docs but I'd like to get some feedback first
  • needs some add. logic to only check the output on non-empty nodes and to ensure multi-output functions are correct
  • no good way for a proper deprecation without a keyword

mathause avatar Feb 10 '25 17:02 mathause

A interpolation use case that doesn't crash with this PR:

import numpy as np

import xarray as xr

number_of_files = 700
number_of_groups = 5
number_of_variables = 10

datasets = {}
for f in range(number_of_files):
    for g in range(number_of_groups):
        # Create random data
        time = np.linspace(0, 50 + f, 1 + 1000 * g)
        y = f * time + g

        # Create dataset:
        ds = xr.Dataset(
            data_vars={
                f"temperature_{g}{i}": ("time", y)
                for i in range(number_of_variables // number_of_groups)
            },
            coords={"time": ("time", time)},
        ).chunk()

        # Prepare for xr.DataTree:
        name = f"file_{f}/group_{g}"
        datasets[name] = ds
dt = xr.DataTree.from_dict(datasets)

# %% Interpolate to same time coordinate
def ds_interp(ds, *args, **kwargs):
    return ds.interp(*args, **kwargs)


new_time = np.linspace(0, 100, 50)
dt_interp = dt.map_over_datasets(
    ds_interp, kwargs=dict(time=new_time, assume_sorted=True)
)

Illviljan avatar Feb 16 '25 09:02 Illviljan

Thanks for the example. This PR would also close #10013. This would be a huge plus for me. Not being able to subtract a ds from a datatree makes it extremely cumbersome. However, this implies that the binary ops are implemented using map_over_datasets and means there is a considerable behavior change.

mathause avatar Feb 17 '25 05:02 mathause

@TomNicholas do you see any chance this PR might get merged (after adding tests etc. obviously)? Are there discussions beside #9693 that I am missing?

mathause avatar Feb 27 '25 17:02 mathause

Hey @mathause - sorry for forgetting about this - I've been busy.

I think something like this should get merged, but there are various small and fairly arbitrary choices to quibble over. They are basically all already mentioned in #9693 though.

this implies that the binary ops are implemented using map_over_datasets and means there is a considerable behavior change.

I don't understand this statement though - aren't binary ops already implemented using map_over_datasets?

https://github.com/pydata/xarray/blob/5ea1e81f6ae7728dd9add2e97807f4357287fa6e/xarray/core/datatree.py#L1590

We're changing the behaviour, but changing it to be closer to the old datatree, which is what a lot of users expect anyway.

TomNicholas avatar Feb 27 '25 18:02 TomNicholas

I think this is ready for review


Hey @mathause - sorry for forgetting about this - I've been busy.

No worries! Thanks for considering this PR!

I think something like this should get merged, but there are various small and fairly arbitrary choices to quibble over. They are basically all already mentioned in #9693 though.

The one unclear choice from #9693 was the comment by @shoyer https://github.com/pydata/xarray/issues/9693#issuecomment-2447896215:

I'm not sure whether or not to call the mapped over function for nodes that only define coordinates. Certainly I would not blindly copy coordinates from otherwise empty nodes onto the result, because those coordinates may no longer be relevant on the result.

I currently use DataTree.has_data, this includes nodes that only have coords (although nodes which inherit coords are excluded (I think)). I don't see a way to be clever about these nodes.

this implies that the binary ops are implemented using map_over_datasets and means there is a considerable behavior change.

I don't understand this statement though - aren't binary ops already implemented using map_over_datasets?

Yes sorry that was not clear. I just wanted to say that binary ops are also affected.

mathause avatar Mar 07 '25 05:03 mathause

gentle ping @TomNicholas

(apologies for bothering you again - I am unfortunately currently blocked by this in another project. Or is there someone else who could potentially review this?)

mathause avatar Mar 13 '25 09:03 mathause

no worries, replied here https://github.com/pydata/xarray/issues/9693#issuecomment-2725117718

TomNicholas avatar Mar 14 '25 16:03 TomNicholas

Any updates on this? #10013 makes binary operations on datatrees quite unfeasible, and a fix would be very welcome!

peanutfun avatar Aug 27 '25 11:08 peanutfun