preserve chunked data when creating DataArray from DataArray

[FabianHofmann]

• Closes preserve chunked data when creating DataArray from itself  #5983
• Tests added
• Passes pre-commit run --all-files

[mathause]

Looks good to me. I can merge in a few days if there are no objections.

You could also add a short note to whats-new - probably under bug fixes.

edit: the test failure is unrelated.

[mathause]

@dcherian you had an opinion on this in #5983 - would you rather raise a ValueError here and advise to pass da.data?

[dcherian]

How about we raise DeprecationWarning for now asking the user to pass in .data instead.

[mathause]

How about we raise DeprecationWarning for now asking the user to pass in .data instead.

Makes sense - how about:

msg = (
    "Passing a `DataArray` object to the `DataArray` constructor is ambiguous.\n"
    "Pass `da.data`, or use `da.copy(deep=True)`, or one of the `xarray.*_like` (e.g. "
    "`xarray.zeros_like`) functions instead,\n"
    "depending on your use case."
)
warnings.warn(msg, FutureWarning)

[max-sixty]

How about we raise DeprecationWarning for now asking the user to pass in .data instead.

Does this mean xr.DataArray(da) would be deprecated? That seems like a very common case. Sorry for joining late but could I ask why this is necessary, if my assumption is right? I had been thinking that is broadly equivalent to a copy.

[keewis]

for reference, the situation we already deprecated and removed was passing (["x", "y"], da) to as_variable (for example, when passed to the coords parameter of DataArray), which would create a new variable with new dimension names, but drop the old attrs and the name / coords and it would convert var to numpy, which could trigger a computation if the data was a dask array (or raise / emit a warning for other array types).

In contrast to that, I think we can keep xr.DataArray(da) working, but only if it's the only argument (i.e. I don't think we should allow xr.DataArray(da, dims=("x", "y")) or something similar – those might still be useful, but I'd make them separate methods instead of overloading the constructor).

However, this might make the constructor a bit more difficult to understand so if we don't think that's worth the (minor) inconvenience:

da if isinstance(da, xr.DataArray) else xr.DataArray(da)

we can go ahead with the deprecation.

[max-sixty]

In contrast to that, I think we can keep xr.DataArray(da) working, but only if it's the only argument (i.e. I don't think we should allow xr.DataArray(da, dims=("x", "y")) or something similar – those might still be useful, but I'd make them separate methods instead of overloading the constructor).

Perfect, very much agree, thank you for the clear description @keewis !

[mathause]

Any objections to merging this as is? If we don't disallow xr.DataArray(da) this is a clear advantage over the current version.

However,

xr.DataArray(da).data is da.data # True
xr.DataArray(da.data).data is da.data # True
# while
da.copy().data is da.data # False (deep=True is the default)

might just be surprising in some situations.

(Just for completeness - xr.DataArray(da, **kwargs) does a good job at erroring if the kwargs are not compatible with da.)

[dcherian]

(Just for completeness - xr.DataArray(da, **kwargs) does a good job at erroring if the kwargs are not compatible with da.)

Sounds good to me then!

[Illviljan]

Thank you, @FabianHofmann !

[FabianHofmann]

It was a pleasure!