CartesianRepresentation objects could be made more powerful #5255

Activity

1. pllim

   added

   Feature Request

   units

   coordinates

   on Aug 23, 2016
2. StuartLittlefair commented on Aug 25, 2016

   

   StuartLittlefair

   on Aug 25, 2016

   ContributorAuthor

   More actions

   After having a brief look at this, it is not quite as trivial as I thought it would be. The issue lies in the codebase for units.Quanitity.

   You can implement __add__ and __radd__ on the CartesianRepresentation class, but the latter will never get called because the Quantity class already has an _add__ which gets called. This should return NotImplemented, but doesn't because Quantity.__array_prepare__ is called first and this raises Exceptions. So in summary:

   a = CartesianRepresentation(1*u.m, 0.5*u.m, 0.0*u.m)
   b = 10*u.m
   sum = a + b

   would work but

   sum = b+a

   would raise a TypeError.
3. mhvk commented on Aug 26, 2016

   

   mhvk

   on Aug 26, 2016

   Contributor

   More actions

   @StuartLittlefair - I think all that would be needed is to define an __array_priority__ that is greater than that of Quantity.

   (Of course, for addition itself, I would hope that it would not work with Quantity at all: which component should the number be added to? If we go ahead with this, we do have to think through carefully what we would like to work; I think it is a relatively limited set.)
4. StuartLittlefair commented on Aug 26, 2016

   

   StuartLittlefair

   on Aug 26, 2016

   ContributorAuthor

   More actions

   I see your point about addition; I was thinking of it as a shift in origin, but on reflection it's not that intuitive and should probably not be allowed. In which case I propose the following changes:

   • CartesianRepresentations can be added and subtracted, but not multiplied?
   • CartesianRepresentations can be multiplied or divided by a Quantity
   • the dot and scalar product of CartesianRepresentations is defined
   • CartesianRepresentation gets a length property.
5. mhvk commented on Aug 27, 2016

   

   mhvk

   on Aug 27, 2016

   Contributor

   More actions

   This sounds good (even if rather than a length property, I'd probably just have __abs__ return the length, just like for complex numbers).

   Note that multiplication/division and abs are meaningful for all representations but UnitSpherical, while dot and perhaps even cross could go through CartesianRepresentation. But I don't think I would want to advocate defining addition and subtraction for any but cartesian coordinates.
6. StuartLittlefair commented on Aug 28, 2016

   

   StuartLittlefair

   on Aug 28, 2016

   ContributorAuthor

   More actions

   Thanks for the comments @mhvk.

   I'm away on holiday for three weeks from next Monday, so I'll probably leave this until after that.
7. mhvk commented on Aug 31, 2016

   

   mhvk

   on Aug 31, 2016

   Contributor

   More actions

   In #5253, we originally introduced an __eq__ for representations, which ended up being taken out. Any follow-up here would be a good place to reintroduce this. I add the actual code below. @eteq had commented on this that

   The point about the equality being the same shape as the representation should be mentioned here (although also see my comment inline).
   Actually, this point should maybe even be added to the narrative docs, because it's behavior that is not really like anything else (it's sensible, but by no means obvious).

       def __eq__(self, other):
           """
           Check for equality with another representation.
           """
           try:
               # this ensures that other can be compared sensibly to self.
               # note that it immediately returns if the classes are the same.
               other = other.represent_as(self.__class__)
           except Exception:
               return False
   
           # Ensure we return a bool or array of bool as appropriate.
           return functools.reduce(np.logical_and,
                                   (getattr(self, component) ==
                                    getattr(other, component)
                                    for component in self.components))
   
       def __ne__(self, other):
           return np.logical_not(self.__eq__(other))
   
8. mhvk commented on Sep 2, 2016

   

   mhvk

   on Sep 2, 2016

   Contributor

   More actions

   See my attempt in #5301. I think it is reasonably complete.
9. StuartLittlefair commented on Nov 16, 2016

   

   StuartLittlefair

   on Nov 16, 2016

   ContributorAuthor

   More actions

   Closing on merge of #5301
10. StuartLittlefair

    closed this as completedon Nov 16, 2016