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.

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?

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.

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`?