issue_comments

I'm also late to the party but I would say I fall squarely in the Dataset is a dict-like camp. If we remove __bool__, should we also remove __len__? Basically, everything @dopplershift aligns with my perspective here.

$0.02 from the peanut gallery is that my mental model of Dataset is that it's a dictionary on steroids, so following the Mapping protocol (including __bool__) makes sense to me. I put "I don't know" for @shoyer's poll, but if the question was "should" then I would have said "number of variables".

While I'm not going to sit here and argue that if ds: is a common operation and important feature (I don't often open files that result in empty datasets), I'd personally want a truly compelling argument for breaking from the Mapping protocol. Right now it's "Dataset works like dict plus some stuff". It would change to be "Dataset works like dict for all things except __bool__, plus some stuff". The latter to me requires more mental effort since I'm no longer strictly supplementing past experience.

I made a Twitter poll, let's see what that says 😄

https://twitter.com/xarray_dev/status/1478776987925684224?s=20

After discussing this a little more, I am on the fence about whether this change this is a good idea. If we can't come to concensus about expected behavior, then that is probably an indication that we should leave things the same to avoid churn.

I do wonder at what point a mapping isn't a mapping anymore?

For example DataFrames aren't considered mappings: python isinstance(df, collections.abc.Mapping) Out[4]: False And if we are to follow pandas example maybe we should just remove the Mapping inheritance? https://github.com/pydata/xarray/blob/60754fdbc4ecd9eb3c0978e82635c6d43e8d485b/xarray/core/dataset.py#L584

TBC, I am fine merging! I would lightly vote against it, but weigh my vote far below @shoyer 's.

And more broadly let's not wait for everyone to agree on everything!

Sorry! I thought we had consensus but perhaps not?

Shall we revert? That said it's PendingDeprecationWarning so it should only be raised in tests, which seems OK?

@dcherian So I guess we decided to go for it? :)

@max-sixty We can still inherit from Mapping and just explicitly raise TypeError inside Dataset.__bool__ rather than using the default implementation. This is certainly a slight violation from the Mapping protocol, but xarray.Dataset is already a little inconsistent (e.g., we override __eq__ inconsistently with the default mapping behavior). Overall I think it's probably a worthwhile the more user-friendly choice?

Would this mean that Dataset doesn't inherit from Mapping (in the end-state, after deprecation)? Or that we inherit from Mapping but override bool to raise an error?

I realize this may be a larger discussion, but the implementation is so easy I went ahead and filed a PR that issues a PendingDeprecationWarning in Dataset.__bool__.

The original intention here was definitely to honor the Mapping protocol exactly, but I agree that bool() is hard to use correctly on Dataset objects. I would support deprecating this.

If there's a synthesis of keeping the truthiness while reducing the chance of these mistakes, that would be very welcome.

@max-sixty im not sure what this would look like. Do you mean a warning or are you hinting that the bar that would need to be met is a silver bullet that preserves bool(ds) but somehow isn’t confusing?

Yeah… I do understand how it’s currently working and why, and the behavior is certainly intuitive to those who appreciate the mapping inheritance.

That said, I feel I have to make a last stand argument because this trips people up quite often (on my team and elsewhere). I haven’t yet come across an example of anyone using this correctly, but I see users misusing it all the time.

The examples and behavior you’re showing @Illviljan seem to me like more the natural result of an implementation detail than a critical principle of the dataset design. While it’s obvious why bool(ds) resolves to True or False in the current implementation, I still think it’s not used and is out of step with how most users think of a dataset. The fact that nothing in the package requires a change in order to pass tests (the broken tests and the pathological case aside) reaffirms for me that this is not a useful feature.

I don’t know much about the mapping protocol or how closely it must be followed. Is the idea here that packages building on xarray (or interoperability features in e.g. numpy or dask) depend on a strict adherence to the full spec?

I definitely empathize with the tradeoff here. That you found xarray's test's were making this error is fairly damning.

But the biggest impediment to changing this behavior is that Dataset follows the Mapping protocol, which has this behavior. One nice feature of xarray is that we follow python's protocols where possible, and that includes truthiness for dict-like / Mapping objects. Notably pd.DataFrame objects only partially implement the protocol, including .values.

If there's a synthesis of keeping the truthiness while reducing the chance of these mistakes, that would be very welcome.

I'm not sure this is an improvement, but in the example converting to a DataArray gets away from the truthiness issue: (result.min(...) >= 1.5).to_array().all()

(I wrote this before seeing @Illviljan 's response, which is very similar)

A Dataset is more similar to a dict or pd.DataFrame.

DataFrame has a similar error, same cooks I suppose: ```python bool(pd.DataFrame()) *** ValueError: The truth value of a DataFrame is ambiguous. Use a.empty, a.bool(), a.item(), a.any() or a.all().

bool(pd.DataFrame([[0, 2], [0, 4]], columns=['A', 'B'])) *** ValueError: The truth value of a DataFrame is ambiguous. Use a.empty, a.bool(), a.item(), a.any() or a.all(). ```

dict works the same way as a dataset, if something exist in it is True: ```python bool({}) False

bool({'a': False}) True `` I see "if not empty do x"-checks all the time with dicts in python code, Is it that strange to follow the behavior ofdict`?