Quantities should always allow comparison with zero by mhvk · Pull Request #1254 · astropy/astropy

mhvk · 2013-10-07T19:32:54Z

Also while working on #929 (where I needed to intercept a where(q==0,...), I noticed that comparisons of the form q==0, q>0 raise an exception unless q is dimensionless. They probably shouldn't as this has a well-defined answer (as 0 can have any unit it wants!). Since quantity is so much in flux right now, I won't do a PR, but just note that one way of simplifying this is by defining a difference function which returns a plain array for testing in the various comparisons. E.g., things can be made to work with.

    def _diff(self, other, raise_on_incompatible=True):
        try:
            return self.value - other.to(self.unit).value
        except AttributeError:
            if np.all(other*0. == other):
                return self.value
            try:
                return self.to(1).value - other
            except(UnitsException,TypeError):
                if raise_on_incompatible:
                    raise TypeError("Quantity object cannot be compared to an "
                                    "object of type {0}"
                                    .format(other.__class__))
        except UnitsException:
            if raise_on_incompatible:
                raise UnitsException("Can only compare quantities with "
                                     "compatible dimensions.")
        return False

(the raise_on_incompatible=False option is there for ==, which should simply return False in this case).

embray · 2013-07-12T20:13:45Z

This sounds reasonable to me, though I'm not sure--could definitely use further discussion.

mdboom · 2013-07-26T20:28:57Z

Yeah -- seems reasonable. We've deliberately excluded offset units (such as Celcius and Fahrenheit would require to convert to Kelvin), so this assertion should hold true.

What's the use case, however? Why not just require the slightly more pedantic case of putting a unit on 0, which gives the added benefit of checking that the unit is compatible?

mhvk · 2013-07-26T21:10:30Z

@mdboom - the use case was that I was trying to make some more numpy functions work, and realised that np.sinc did not work because internally it does an np.where(q==0,...) (to avoid a division by zero), which throws an error when comparing q to 0 (see first message of this issue). I then thought the comparison is generally valid, so we might as well allow it. Indeed, it temporarily entered the general unit check for two-argument ufunc's, and could trivially go back there. But I also see the logic of just being a bit more pedantic and insisting on 0 also having units until a better use case pops up.

mhvk · 2013-09-21T02:23:33Z

Still feel this is sensible, but no hurry.

taldcroft · 2013-09-22T21:23:34Z

Since this isn't a priority for 0.3, I'm reassigning to 0.4.

astrofrog · 2013-10-04T09:16:01Z

After implementing support for astropy.units into one of my codes, I can definitely see the value for this and also for comparing with unitless np.inf and -np.inf. If feel silly to have to do e.g.

q[2] = 0. * u.m
q[3] = np.inf * u.cm

since the units are irrelevant.

mhvk · 2013-10-07T19:35:47Z

@astrofrog - I still had my "zero can have arbitrary units" PR around, so now updated it to include inf and nan. With this, your examples work. The PR includes some test cases for comparisons, but not much beyond it. PRs to my PR most welcome.

mhvk · 2013-10-18T02:21:22Z

@astrofrog - no hurry, but if you have time to look at this comparison-with-zero/inf PR, that would be great.

astrofrog · 2013-10-18T12:05:25Z

Will do later today hopefully, and @mdboom should look too.

mdboom · 2013-10-18T12:12:24Z

It might make more sense to include np.all here, rather than place the burden on the callers of this function.

Also, maybe add a docstring. (I know it's private, but it might not be clear what its purpose is a few months from now).

mdboom · 2013-10-18T12:16:38Z

Looks good, aside from my minor comments.

mhvk · 2013-10-18T12:57:03Z

@mdboom - thanks, all comments implemented.

astrofrog · 2013-10-21T15:31:09Z

@mhvk - can you explain these changes?

Zero has become special with this PR: previously, the assertion was correct since c.x[0], which has units of kpc, is not equal to 0, which does not have these units. However, with the present PR, 0 can have any units, and thus one does have c.x[0] == 0

(I must admit that I'm not 100% sure that the above was the intent of the test...)

…a test

mhvk · 2013-11-03T01:23:12Z

@astrofrog, @mdboom - looking over old branches, I encountered this one. I think I had all your comments addressed, so it should be good to go. (I rebased on current master to make it mergeable, so travis will run once more as well.)

Quantities should always allow comparison with zero

embray · 2013-11-05T15:45:46Z

If this is merged shouldn't it be 0.3?

mdboom · 2013-11-05T20:53:40Z

Yes, 0.3.

mhvk mentioned this pull request Jul 12, 2013

Refactor Quantity into Numpy ndarray sub-class #929

Merged

mdboom reviewed Oct 18, 2013
View reviewed changes

astrofrog reviewed Oct 21, 2013
View reviewed changes

mhvk added 4 commits November 2, 2013 21:18

Reallow comparison with 0 even for dimensional quantities

117d573

And also treat inf and nan as quantities that can have arbitrary units

4f32fcd

Ensure coordinates.tests.test_distance does not fail

1825dfe

Document _can_have_arbitrary units and apply to all members; add extr…

689d5ae

…a test

mdboom added a commit that referenced this pull request Nov 4, 2013

Merge pull request #1254 from mhvk/units-zero-always-ok

9acd4cf

Quantities should always allow comparison with zero

mdboom merged commit 9acd4cf into astropy:master Nov 4, 2013

mhvk deleted the units-zero-always-ok branch November 4, 2013 21:26

mdboom mentioned this pull request Sep 19, 2014

Setting Quantity item inconsistent #2959

Closed

embray removed the Ready-for-final-review label Jan 13, 2015

mhvk mentioned this pull request Aug 11, 2020

ENH: Speed up trim_zeros numpy/numpy#16911

Merged

mhvk mentioned this pull request Aug 11, 2023

Quantity comparison to dimensionless zero has an amusing edge case #15103

Closed

Conversation

mhvk commented Oct 7, 2013

Uh oh!

embray commented Jul 12, 2013

Uh oh!

mdboom commented Jul 26, 2013

Uh oh!

mhvk commented Jul 26, 2013

Uh oh!

mhvk commented Sep 21, 2013

Uh oh!

taldcroft commented Sep 22, 2013

Uh oh!

astrofrog commented Oct 4, 2013

Uh oh!

mhvk commented Oct 7, 2013

Uh oh!

mhvk commented Oct 18, 2013

Uh oh!

astrofrog commented Oct 18, 2013

Uh oh!

mdboom Oct 18, 2013

Choose a reason for hiding this comment

Uh oh!

mdboom commented Oct 18, 2013

Uh oh!

mhvk commented Oct 18, 2013

Uh oh!

astrofrog Oct 21, 2013

Choose a reason for hiding this comment

Uh oh!

mhvk Oct 21, 2013

Choose a reason for hiding this comment

Uh oh!

mhvk commented Nov 3, 2013

Uh oh!

embray commented Nov 5, 2013

Uh oh!

mdboom commented Nov 5, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants