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

Already on GitHub? Sign in to your account

Coordinates equality comparison fails for array values #1832

Merged
merged 4 commits into from Feb 27, 2014

Conversation

Projects
None yet
3 participants
Owner

eteq commented Feb 11, 2014

In [1]: from astropy.coordinates import ICRS, Angle
In [2]: c1 = ICRS([1,2], [3,4], unit=('deg', 'deg'))
In [3]: c1 == c1
ERROR: ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all() [astropy.coordinates.coordsystems]
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-3-5768a5d8dcd9> in <module>()
----> 1 c1 == c1

/data/baffin/tom/git/astropy/astropy/coordinates/coordsystems.pyc in __eq__(self, other)
     50     def __eq__(self, other):
     51         try:
---> 52             return (self.latangle == other.latangle and
     53                     self.lonangle == other.lonangle and
     54                     self.distance == other.distance and

ValueError: The truth value of an array with more than one element is ambiguous. Use a.any() or a.all()

Needs np.all() around the comparisons.

Owner

eteq commented Feb 10, 2014

@embray - this is trivial enough that it probably should be fixed for 0.3.1 ...? I'll try to get to this ASAP, but it really is probably as sample as inserting np.all in __eq__ for the coordinate objects.

Owner

eteq commented Feb 11, 2014

Alright, the attached code should do the trick @taldcroft. We may want to consider when equality should actually work for the APE5 architecture, but this is definitely a bug as it works right now.

@embray, assuming the tests pass, you can just go ahead and merge this if you're triaging 0.3.1 issues.

Owner

taldcroft commented Feb 11, 2014

@eteq - Should you be checking that the coordinate system (and other relevant metadata like obs_time) is the same? This is a slightly deeper question of what is meant by equality, and technically out of scope for this issue, but this seems wrong to me:

>>> c1 = ICRS(1, 2, unit=['deg','deg'])
>>> c2 = FK5(1, 2, unit=['deg','deg'])
>>> c1 == c2
True
Owner

taldcroft commented Feb 11, 2014

One more little thing:

>>> c3 = Galactic(1, 2, unit=['deg','deg'])
>>> c1 == c3
ERROR: OperandTypeError [astropy.time.core]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/aldcroft/git/astropy/astropy/coordinates/coordsystems.py", line 55, in __eq__
    self.equinox == other.equinox)
  File "/Users/aldcroft/git/astropy/astropy/time/core.py", line 954, in __eq__
    return self._tai_difference(other) == 0.
  File "/Users/aldcroft/git/astropy/astropy/time/core.py", line 942, in _tai_difference
    raise OperandTypeError(self, other)
astropy.time.core.OperandTypeError

Not sure if you want to make a new issue or just fix it here by trying the equinox test and failing equality if there was an exception.

Owner

taldcroft commented Feb 11, 2014

Or maybe this is a bug in time?

CHANGES.rst
@@ -231,6 +231,9 @@ Bug Fixes
- ``astropy.coordinates``
+ - Fixed a bug where using `==` on two array coordinates wouldn't
+ work. [#1832]
@embray

embray Feb 14, 2014

Member

Same nag I made to @larrybradley re #2098 :)

@embray embray modified the milestones: v0.3.2, v0.3.1 Feb 25, 2014

Owner

eteq commented Feb 27, 2014

@taldcroft - I see your point that FK5 and ICRS should not be equal in this case. But I think that's too big of a change to count as a "bug fix", so it will be easier to address this once the APE5 re-factoring is in place (right now there's not a "standard" way to check that two systems are the same because the metadata isn't all the same). So I agree with you that it should be fixed to also check all those framre-related attributes things, but I think it's better to do it in 0.4 when it will be easier and go with a bunch of other changes.

As for the error you saw with Galactic, I see the same thing, but I think this comes from behavior in time even absent anything to do with coordinates (see test case below). I would say <time object> == None should return False instead of raising an exception?

So @taldcroft, if you agree that these can be dealt with separately, feel free to merge this, as the tests pass now. You can create issues for both of the other items (or just ping me and I can make them).

In [19]: t=Time('J2000',scale='utc')
In [20]: t==None
---------------------------------------------------------------------------
OperandTypeError                          Traceback (most recent call last)
<ipython-input-20-41bd8a8f7385> in <module>()
----> 1 t==None

/Users/erik/src/astropy/build/lib.macosx-10.9-x86_64-2.7/astropy/time/core.pyc in __eq__(self, other)
    952 
    953     def __eq__(self, other):
--> 954         return self._tai_difference(other) == 0.
    955 
    956     def __ne__(self, other):

/Users/erik/src/astropy/build/lib.macosx-10.9-x86_64-2.7/astropy/time/core.pyc in _tai_difference(self, other)
    940                 other = self.__class__(other)
    941             except:
--> 942                 raise OperandTypeError(self, other)
    943         self_tai = self.tai
    944         other_tai = other.tai


Owner

taldcroft commented Feb 27, 2014

@eteq - this looks good for the original issue. The t == None problem is addressed by #2114 (which I think is ready to merge but now needs to be rebased). I'll open the issue on comparing coordinates in different systems.

taldcroft added a commit that referenced this pull request Feb 27, 2014

Merge pull request #1832 from eteq/fix-1832
Coordinates equality comparison fails for array values

@taldcroft taldcroft merged commit a4d082e into astropy:master Feb 27, 2014

taldcroft added a commit that referenced this pull request Mar 27, 2014

Merge pull request #1832 from eteq/fix-1832
Coordinates equality comparison fails for array values

mdmueller added a commit to mdmueller/astropy that referenced this pull request Apr 2, 2014

mdmueller added a commit to mdmueller/astropy that referenced this pull request Apr 2, 2014

ktchrn added a commit to ktchrn/astropy that referenced this pull request Oct 28, 2014

ktchrn added a commit to ktchrn/astropy that referenced this pull request Oct 28, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment