Skip to content

Conversation

@mhvk
Copy link
Contributor

@mhvk mhvk commented Nov 30, 2013

While working on #1839, I realised Longitude._wrap_angle was not always updated whenever _unit was, and I tracked this down to a missing __array_finalize__; with that in place, __getitem__ and __getslice__ where no longer needed, so it simplified matters. Also changed how numpy scalars are handled: view them as an array using __array__() and then turn the view back into a Quantity.

EDIT -- updated title to reflect that I also changed CartesianPoints (see below)

@mhvk
Copy link
Contributor Author

mhvk commented Nov 30, 2013

My initial changes to the internal use of __quantity_view__, __quantity_instance__ and __array_wrap__ in Quantity caused problems in Coordinates and upon closer inspection I noticed they problems all were due to these methods being used incorrectly in CartesianPoints. In particularly, unlike what one might infer from the name, __quantity_instance__ and __quantity_view__ are not meant to return Quantity instances/views of oneself, but rather to allow one to fall back to one's superclass if desired. It was more or less invented for Angle, where if a unit is no longer an angular one, a Quantity is returned, as shown below. @eteq - I'm fairly sure my corrections to CartesianPoints are correct, but please check.

@mdboom, @astrofrog - we may need to change the names of these methods. It may be worth thinking a bit more generally if it is the right approach.

    def __quantity_view__(self, obj, unit):
        unit = self._convert_unit_to_angle_unit(unit)
        if unit is not None and unit.is_equivalent(u.radian):
            result = obj.view(self.__class__)
            return result
        return super(Angle, self).__quantity_view__(
            obj, unit)

    def __quantity_instance__(self, val, unit, **kwargs):
        unit = self._convert_unit_to_angle_unit(unit)
        if unit is not None and unit.is_equivalent(u.radian):
            return self.__class__(val, unit, **kwargs)
        return super(Angle, self).__quantity_instance__(val, unit, **kwargs)

@mhvk
Copy link
Contributor Author

mhvk commented Nov 30, 2013

travis failures are due to timeouts; can ping travis once people have commented.

@mhvk
Copy link
Contributor Author

mhvk commented Dec 3, 2013

@mdboom - I realised my chain of PRs was hopeless for making any changes, and so pulled it apart. This one now only addresses better use of __array_finalize__ and __array_wrap__ in Quantity and its subclasses. I implemented an extra comment you asked about in #1851, and used str() in the dtype definition.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure about this, but I think this is intended behavior - that is, if you apply a unfunc to a cartesian point, and its unit is not a length, then we don't want it to go through - then it's not a position anymore.

It may be this is explicitly checked for before __array_wrap__, or this was intended to be the check, but either way I think this change isn't necessary/intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main problem here was calling the initialiser, which does not belong in __array_wrap__ (but rather should be the domain of __quantity_view__ and __quantity_instance__.

But the behaviour also seemed odd: instead of raising an exception, why not fall back to Quantity for calculations that do not give a length (e.g., suppose I want a velocity, why stop me from doing point / (1.*u.yr)?). This is what we do for Angle and its various subclasses as well, and seems least restrictive, so I tried to mimic that behaviour here (with the actual decision on what class to instantiate happening in __array_wrap__, like for Angle, etc.)

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, I understand what you're doing now - thanks for the clarification.

On the final point, I see your point, and I think I'm OK with it.

The one place where it's really important is in initialization, though: we definitely want an exception if, for example, someone does CartesianPoints(xarr*u.Mpc, yarr*u.Mpc,zrr*u.Mpc/u.Gyr), rather than having it silently fall back to a Quantity. But I tested it right now, and that does indeed raise an exception, so I think this is fine.

@eteq
Copy link
Member

eteq commented Dec 6, 2013

I'm inferring from some of the comments you made above that @mdboom said something about this, but I can't see what... Your changes to CartesianPoints do make sense based on the explanation here, but it would be good if @mdboom could glance at them to make sure that's what was inteded for those methods.

@mhvk
Copy link
Contributor Author

mhvk commented Dec 6, 2013

@eteq - the __quantity_view__ and __quantity_instance__ go back to when we made Quantity an ndarray subclass (#929). But it would be great to have others look at it; this was a somewhat unexpected deep-end plunge.

@mdboom
Copy link
Contributor

mdboom commented Dec 10, 2013

Makes sense to me.

@mhvk
Copy link
Contributor Author

mhvk commented Dec 10, 2013

Just to be sure, since quite a few Quantity PRs were merged, I rebased. Seems all OK.

@mhvk
Copy link
Contributor Author

mhvk commented Dec 12, 2013

@eteq - does my explanation of the changes make sense? Do you think this is OK to go in? Longer-term, may need to reconsider the set-up (see #1850), but at least this should make things a bit more robust.

@eteq
Copy link
Member

eteq commented Dec 12, 2013

Sure, I think this is fine, @mhvk - will go ahead and merge it now. We can have further discussion on more general points in #1850 and the upcoming coordinates APE/API.

eteq added a commit that referenced this pull request Dec 12, 2013
Better use of __array_finalize__ and __array__ in quantity/longitude/CartesianPoints
@eteq eteq merged commit c618d25 into astropy:master Dec 12, 2013
@mhvk mhvk deleted the quantity-improved-array-views branch September 26, 2014 16:39
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.

3 participants