Skip to content

bool(Dataset(False)) is True #4290

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aaronspring opened this issue Jul 30, 2020 · 9 comments
Closed

bool(Dataset(False)) is True #4290

aaronspring opened this issue Jul 30, 2020 · 9 comments

Comments

@aaronspring
Copy link
Contributor

aaronspring commented Jul 30, 2020

What happened:

v=True
bool(xr.DataArray(v)) # True
bool(xr.DataArray(v).to_dataset(name='var')) # True

v=False
bool(xr.DataArray(v)) # False
# unexpected behaviour below
bool(xr.DataArray(v).to_dataset(name='var')) # True

What you expected to happen:

bool(xr.DataArray(False).to_dataset(name='var')) # False

Maybe this is intentional and I dont understand why.

xr.version = '0.16.0'

@keewis
Copy link
Collaborator

keewis commented Jul 30, 2020

If I remember correctly, Dataset inherits from dict and doesn't define it's own __bool__, so it has the behavior of dict.__bool__:

In [2]: bool(xr.Dataset())
Out[2]: False

In [3]: bool(xr.Dataset({"x": False}))
Out[3]: True

In [4]: bool({})
Out[4]: False

In [5]: bool({"x": False})
Out[5]: True

@aaronspring
Copy link
Contributor Author

thanks for the explanation. but from a naive user perspective,

bool(xr.DataArray(False).to_dataset(name='var')) # False
bool(xr.DataArray(True).to_dataset(name='var')) # True
bool(xr.DataArray(False)) # False
bool(xr.DataArray(True)) # True

would be nice.

@keewis
Copy link
Collaborator

keewis commented Jul 30, 2020

I'm not sure if that's something we want to do: what happens if the dataset has more than one variable?

I guess what we're talking about here is making bool(ds) a alias of all(ds.values() or [False]) (the ... or [False] is for making Dataset({}) return False instead of True).

Thoughts, @pydata/xarray?

@aaronspring
Copy link
Contributor Author

aaronspring commented Jul 30, 2020

currently

if (xrobject > 10).any():
   raise ValueError('blabla')

depends on whether xrobject is xr.DataArray or xr.Dataset.

@TomNicholas
Copy link
Member

This question seems to me to be conceptually similar to the question I described in #3315: "Should we treat a named DataArray and a single-variable Dataset as if they are the same?"

If the answer is yes, then we should definitely change the behaviour of bool(Dataset()) as Aaron suggests. If instead we decide "no, Datasets are dictionaries of DataArrays, and a single-element dictionary is not the same thing as the named object it contains", then I'm not sure.

Intuitively I personally think it should be the former, but I would first like to see how many tests break if we make that kind of change.

@max-sixty
Copy link
Collaborator

We mirror numpy for DataArray and, per @keewis , Mapping for Dataset. I think we should have a fairly high bar for deviating from those.

For those who think bool(xr.Dataset(dict(x=False))) should evaluate to False, what do you think dict(x=False) should evaluate to?

IIUC, this is somewhat of an edge case — it's only possible to coerce DataArray to bool when there's exactly one element. (Not that we shouldn't resolve)

This question seems to me to be conceptually similar to the question I described in #3315: "Should we treat a named DataArray and a single-variable Dataset as if they are the same?"

👍

@dcherian
Copy link
Contributor

We mirror numpy for DataArray and, per @keewis , Mapping for Dataset. I think we should have a fairly high bar for deviating from those.

👍

@aaronspring
Copy link
Contributor Author

For those who think bool(xr.Dataset(dict(x=False))) should evaluate to False, what do you think dict(x=False) should evaluate to?

good point

IIUC, this is somewhat of an edge case — it's only possible to coerce DataArray to bool when there's exactly one element. (Not that we shouldn't resolve)

I now understand the underlying problem better with your comments. My expectations were too naive about it. I cannot evaluate the consequences of changing this. I close this issue and hopefully users having this problem in the future will find this issue.

@max-sixty
Copy link
Collaborator

Thanks for raising and engaging @aaronspring !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants