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

Implemented Quantity-aware wrappers for assert_allclose and linspace #2402

Closed
wants to merge 1 commit into from

Conversation

astrofrog
Copy link
Member

As discussed in #1274, it would be great to have Quantity-aware wrappers to some Numpy functions. We don't have to monkey-patch these, we can just provide them in a separate namespace. This PR can be used e.g. as:

In [2]: from astropy import units as u

In [3]: from astropy.units import numpy_wrappers as unp

In [4]: unp.linspace(1 * u.cm, 1. * u.m, 100)
Out[4]: 
<Quantity [   1.,   2.,   3.,   4.,   5.,   6.,   7.,   8.,   9.,  10.,
             11.,  12.,  13.,  14.,  15.,  16.,  17.,  18.,  19.,  20.,
             21.,  22.,  23.,  24.,  25.,  26.,  27.,  28.,  29.,  30.,
             31.,  32.,  33.,  34.,  35.,  36.,  37.,  38.,  39.,  40.,
             41.,  42.,  43.,  44.,  45.,  46.,  47.,  48.,  49.,  50.,
             51.,  52.,  53.,  54.,  55.,  56.,  57.,  58.,  59.,  60.,
             61.,  62.,  63.,  64.,  65.,  66.,  67.,  68.,  69.,  70.,
             71.,  72.,  73.,  74.,  75.,  76.,  77.,  78.,  79.,  80.,
             81.,  82.,  83.,  84.,  85.,  86.,  87.,  88.,  89.,  90.,
             91.,  92.,  93.,  94.,  95.,  96.,  97.,  98.,  99., 100.] cm>

I'd like to include a few functions like this in 0.4 if there are no objections.

@astrofrog
Copy link
Member Author

I can write docs if there are no objections to including this.

@astrofrog astrofrog added the units label May 1, 2014
@astrofrog astrofrog added this to the v0.4.0 milestone May 1, 2014
@adrn
Copy link
Member

adrn commented May 1, 2014

I like it!

:func:`numpy.testing.assert_allclose`.
"""
if isinstance(actual, Quantity) and isinstance(desired, Quantity):
np.testing.assert_allclose(actual.value, desired.to(actual.unit).value,
Copy link
Member

Choose a reason for hiding this comment

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

I had written a similar utility function for testing and called it assert_quantity but with different semantics.

If you write it like that, then at least you have to describe in the docstring that you convert desired to actual and then e.g. atol applies to those numbers.

There's quite a bit of tests in Astropy already where asserts on quantities are split in two lines ... one testing values and the other testing units ... maybe assert_quantity makes sense in addition and you leave this one as is?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I guess there are two use-cases - one where you check if the values are equivalent, and one where you check where they are exactly equal.

Copy link
Member

Choose a reason for hiding this comment

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

And I guess users could also expect quantities for atol to work?

I think this shows that it's often not 100% straight-forward how to write a quantity-aware wrapper and we have to look at each one and make sure the docstring explains what it really does.

@cdeil
Copy link
Member

cdeil commented May 1, 2014

I don't know if this is a good idea. Will make some code a bit shorter, but also has the potential for confusion (accidentally use the wrong linspace).

@astrofrog If we do add it, could you explicity recommend the import convention you have proposed above

from astropy.units import numpy_wrappers as unp
unp.linspace(...)

and say that this should be avoided

from astropy.units.numpy_wrappers import linspace

because it can lead to confusion with code that uses

from numpy import linspace

?

There's the uncertainties package that has the unumpy package where "u" means uncertainty. It's probably not a big deal, but maybe "qnp" ("q" for "quantity") might be less confusing (especially if users use astropy quantities and nduncertainties)?

"""
if isinstance(start, Quantity) and isinstance(stop, Quantity):
return np.linspace(start.value, stop.to(start.unit).value, num=num,
endpoint=endpoint, retstep=retstep, dtype=dtype) * start.unit
Copy link
Member

Choose a reason for hiding this comment

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

In my numpy, linspace doesn't accept a dtype option!?

In [7]: np.linspace(1, 10, dtype=float)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-7-8979a0b673b3> in <module>()
----> 1 np.linspace(1, 10, dtype=float)

TypeError: linspace() got an unexpected keyword argument 'dtype'

In [8]: np.__version__
Out[8]: '1.8.1'

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so maybe arguments that don't care about quantities should be passed via **kwargs to avoid this kind of issue.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed--it should just work as a transparent replacement for the Numpy ones, keeping in mind that the signatures for some of these functions have changed between Numpy versions :/

@mhvk
Copy link
Contributor

mhvk commented May 1, 2014

I'm not very keen on this, unless it is really obvious that one couldn't change the numpy functions to work this way (and then work via utils.compat.numpy). Is this difficult for np.linspace? A quick look at numpy/core/function_base.py suggests it would be quite trivial to addFor assert_allclose, I can see that numpy would have little interest in having their test routines touched, but then I think it is more logical to define a new function (as suggested by @cdeil).

@mhvk
Copy link
Contributor

mhvk commented May 1, 2014

Actually..., it seems at least with python3 and numpy 1.9.0-dev, np.linspace works already!!!

n [1]: import astropy.units as u; import numpy as np

In [2]: np.linspace(0.*u.m, 10.*u.m, 10)
Out[2]: 
<Quantity [  0.        ,  1.11111111,  2.22222222,  3.33333333,
             4.44444444,  5.55555556,  6.66666667,  7.77777778,
             8.88888889, 10.        ] m>
In [3]: np.linspace(0.*u.m, 10.*u.s, 10)
ERROR: UnitsError: Can only apply 'subtract' function to quantities with compatible dimensions [astropy.units.quantity_helper]

@astrofrog
Copy link
Member Author

@mhvk - huh, you're right - even with compatible but different units:

In [4]: np.linspace(1 * u.cm, 1. * u.m, 100)
Out[4]: 
<Quantity [ 0.01, 0.02, 0.03, 0.04, 0.05, 0.06, 0.07, 0.08, 0.09, 0.1 ,
            0.11, 0.12, 0.13, 0.14, 0.15, 0.16, 0.17, 0.18, 0.19, 0.2 ,
            0.21, 0.22, 0.23, 0.24, 0.25, 0.26, 0.27, 0.28, 0.29, 0.3 ,
            0.31, 0.32, 0.33, 0.34, 0.35, 0.36, 0.37, 0.38, 0.39, 0.4 ,
            0.41, 0.42, 0.43, 0.44, 0.45, 0.46, 0.47, 0.48, 0.49, 0.5 ,
            0.51, 0.52, 0.53, 0.54, 0.55, 0.56, 0.57, 0.58, 0.59, 0.6 ,
            0.61, 0.62, 0.63, 0.64, 0.65, 0.66, 0.67, 0.68, 0.69, 0.7 ,
            0.71, 0.72, 0.73, 0.74, 0.75, 0.76, 0.77, 0.78, 0.79, 0.8 ,
            0.81, 0.82, 0.83, 0.84, 0.85, 0.86, 0.87, 0.88, 0.89, 0.9 ,
            0.91, 0.92, 0.93, 0.94, 0.95, 0.96, 0.97, 0.98, 0.99, 1.  ] m>

@mhvk
Copy link
Contributor

mhvk commented May 2, 2014

@astrofrog - for np.linspace, I guess we should find out what numpy versions are not OK (maybe just run your tests on travis using the numpy version). If there are versions for which it does not work, I think this would be a good addition to utils.compat.numpy.

For assert_allclose, I would still suggest first looking at the numpy version to see whether it would involve a trivial change to get it to work. If not, I'm in favour of @cdeil's assert_quantity (i.e., another name).

@astrofrog
Copy link
Member Author

It sounds like we may need to think about this more, so we'll definitely miss the feature freeze. I'm re-scheduling to 1.0.

@mhvk
Copy link
Contributor

mhvk commented Jan 13, 2015

This will get closed by #3273, so leaving the milestone.

@astrofrog
Copy link
Member Author

Superseded by #3273

@astrofrog astrofrog closed this Jan 15, 2015
@astrofrog astrofrog deleted the initial-numpy-wrappers branch July 5, 2016 18:46
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.

None yet

5 participants