Skip to content

Conversation

@keewis
Copy link
Contributor

@keewis keewis commented Jun 21, 2020

When comparing quantities with different units / dimensionalities but containing duck arrays for equality, the returned value tends to change between boolean arrays and python boolean objects depending on the input. This makes it somewhat difficult to write code / tests against it and with this I'd propose to change that to always return arrays if we're using array magnitudes.

I'm not quite sure where bool_result should live, maybe compat.py?

Comment on lines +1505 to +1508

# TODO: this might be expensive. Do we even need it?
if eq(self._magnitude, 0, True) and eq(other._magnitude, 0, True):
return self.dimensionality == other.dimensionality
return bool_result(self.dimensionality == other.dimensionality)
Copy link
Contributor Author

@keewis keewis Jun 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure out what this block is for. I tried removing the whole block and rerun the tests, but this didn't break any tests. Since this is potentially expensive (we might end up comparing the values multiple times on huge arrays) I'd suggest removing this:

Suggested change
# TODO: this might be expensive. Do we even need it?
if eq(self._magnitude, 0, True) and eq(other._magnitude, 0, True):
return self.dimensionality == other.dimensionality
return bool_result(self.dimensionality == other.dimensionality)

@hgrecco
Copy link
Owner

hgrecco commented Jun 21, 2020

While I am in principle ok about making the output more predictable and easy to understand, I think it would be good to put here (or in an issue) some examples of current and expected behavior so more people can be aware of this (breaking) change.

@keewis
Copy link
Contributor Author

keewis commented Jun 21, 2020

okay, then here are the examples (mostly from the test cases):

u = np.ones((10,)) * ureg.dimensionless
v = np.zeros_like(u) * ureg.m
w = np.ones_like(u) * ureg.m

# compare a quantity with itself
u == u  # array-like, same as before
# compare dimensionless with non-quantity
u == 1  # array-like, before this was a single bool
# compare quantity with dimensionality with non-quantity
v == 1  # array-like, before this was a single bool

# compare quantities with different units and with the values being all 0
np.zeros_like(u) * ureg.m == np.zeros_like(u) * ureg.s
# array-like, before this returned a single bool

# compare two quantities with equal units
v == w  # array-like, same as before
# compare two quantities with compatible units
v == w.to("mm")  # array-like, same as before
# compare quantities with different units and different values
u == v  # array-like, before this was a single bool

as a summary: if the other quantity has a different unit the behaviour changed. Before it returned a single bool, now it returns an array filled with that bool.

which exposed a bug in the bool_result function
@hgrecco
Copy link
Owner

hgrecco commented Jul 8, 2020

This looks good to me and there has not been any comment. I am merging so it can be included in the next release

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 8, 2020

Build succeeded:

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

Successfully merging this pull request may close these issues.

Array comparisons return a single boolean if arrays are 0

2 participants