Skip to content
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

Remove Quantity.__eq__, __ne__ so that == and != always return True, False #2328

Merged
merged 3 commits into from
Apr 21, 2014

Conversation

mhvk
Copy link
Contributor

@mhvk mhvk commented Apr 16, 2014

Currently, q1==q2 and q1!=q2 raise UnitsError if the units do not match. This, however, would seem overly protective of the user, and inconsistent with python usage elsewhere, where == always returns False is objects are incompatible. Simply removing the __eq__ and __ne__ from Quantity, and thus letting ndarray take care, solves this.

As a byproduct, one now gets the same result for unit == (1.*unit) and (1.*unit) == unit, since Quantity returns NotImplemented and Unit decides that it can turn the quantity into a unit (arguably, it shouldn't, but that is a problem for Unit, and at least it should be symmetric!).

@mhvk
Copy link
Contributor Author

mhvk commented Apr 16, 2014

p.s. An additional advantage is that Quantity holding structured arrays behave better, with == and != both converting units properly.

@eteq
Copy link
Member

eteq commented Apr 17, 2014

Yeah, I guess it makes sense for this to follow the more common behavior, so it seems fine to me.

You should add this to "backwards-incompatible changes" in the 0.4 what's new document, though, as it's a potential "gotcha" for legacy code.

@eteq
Copy link
Member

eteq commented Apr 17, 2014

oh, but it would be good to hear from @adrn, @astrofrog, and/or @mdboom or this one...

@mdboom
Copy link
Contributor

mdboom commented Apr 17, 2014

You can have a Quantity storing a structured array? I thought we explicitly only allow float and complex arrays?

@mdboom
Copy link
Contributor

mdboom commented Apr 17, 2014

This looks good. One other thing this fixes (and we should add an explicit test for) is:

assert 100 * u.cm == u.m

Before, one would have to explicitly fall back on Unit comparison:

u.Unit(100 * u.cm) == u.m

@mhvk
Copy link
Contributor Author

mhvk commented Apr 17, 2014

You can have a Quantity storing a structured array? I thought we explicitly only allow float and complex arrays?

No, it is not explicitly excluded (and I am much in favour of not
excluding anything remotely sensible...). Though since ufuncs cannot
do much with structured arrays, it is quite handicapped.

@mdboom
Copy link
Contributor

mdboom commented Apr 17, 2014

I guess I'm missing how a structured array in a Quantity is sensible. What's the use case?

@mhvk
Copy link
Contributor Author

mhvk commented Apr 17, 2014

@mdboom - so far, I've only been thinking about a vector, where the components would be in a structured array. But it is not obvious that this is handier than having x,y,z components in the last dimension. Anyway, this PR is not specifically for that.

@mhvk
Copy link
Contributor Author

mhvk commented Apr 18, 2014

@eteq: I added an entry to the backwards-incompatible changes section; @mdboom - I added the test.
Merge if happy.

@astrofrog
Copy link
Member

👍

@eteq
Copy link
Member

eteq commented Apr 18, 2014

👍 from me too, so if @mdboom is happy, feel free to merge

@mhvk mhvk added the units label Apr 20, 2014
@mhvk mhvk added this to the v0.4.0 milestone Apr 20, 2014
@mhvk
Copy link
Contributor Author

mhvk commented Apr 21, 2014

@mdboom - OK to merge? If so, I can remove __eq__ and __ne__ in EarthLocation...

with pytest.raises(u.UnitsError):
u.Quantity(1, unit='m') == u.Quantity(1, unit='s')
# for ==, !=, return False, True if units do not match
assert u.Quantity(1100, unit=u.m) != u.Quantity(1, unit=u.s)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's important that we maintain a subtle feature of this fix. If the units are convertible, we get an array of results (and it's a vectorized array comparison), and if the units are not convertible, NotImplemented is raised and the result is False

In [4]: 3.0 * u.s == 3.0 * u.m
Out[4]: False

In [5]: 3.0 * u.m == 280.0 * u.cm
Out[5]: array(False, dtype=bool)

So maybe this line could be:

assert (u.Quantity(1100, unit=u.m) != u.Quantity(1, unit=u.s)) is True
assert (u.Quantity(1100, unit=u.m) == u.Quantity(1, unit=u.s)) is False

@mdboom
Copy link
Contributor

mdboom commented Apr 21, 2014

Other than my comment about tightening up the tests, I think this is good to go.

@mhvk
Copy link
Contributor Author

mhvk commented Apr 21, 2014

@mdboom - thanks, implemented. As it passed travis just now, I'll merge.

mhvk added a commit that referenced this pull request Apr 21, 2014
Remove Quantity.__eq__, __ne__ so that == and != always return True, False
@mhvk mhvk merged commit 621f956 into astropy:master Apr 21, 2014
@mhvk mhvk deleted the quantity-equality branch April 21, 2014 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants