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

Add a new decorator for functions accepting quantities. #3072

Merged
merged 23 commits into from Dec 17, 2014

Conversation

@Cadair
Copy link
Member

Cadair commented Nov 5, 2014

This is something that has cropped up in a discussion inside SunPy about how to verify the inputs on functions which expect quantities.

We often have the situation where the input to the function is a quantity (or many) and because it is representing something physical, it is expected to be convertible to a specific (usual) unit. i.e. convertible to degrees for a on sky angle. This decorator will check all arguments to a function and make sure that they are convertible to the specified units and raise a nice error if they are not.

The decorator expects a number of arguments equal to the number of arguments to the function and any keyword arguments that need to be verified.

i.e.

@quantity_input(u.deg, u.deg, None, distance=u.km)
def myfunction(ra, dec, someflag, distance=1*u.au):
     ra.unit == u.deg
     dec.unit == u.deg

p.s. I am aware that this is not the place for this function in the code base and it needs tests etc, but I thought I would first solicit opinions about if it belongs in astropy at all.

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Nov 5, 2014

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Nov 5, 2014

@mdboom

This comment has been minimized.

Copy link
Contributor

mdboom commented Nov 6, 2014

This is really what function annotations were made for:

http://legacy.python.org/dev/peps/pep-3107/

Too bad we can't rely exclusively on Python 3 yet...

In the 2/3 world we live in, this does look handy.

try:
args[i] = args[i].to(f_arg)
except UnitsError:
raise TypeError("Argument '{}' to function '{}' must be in units convertable to '{}'.".format(f.func_code.co_varnames[i], f.func_code.co_name, f_arg.to_string()))

This comment has been minimized.

Copy link
@mdboom

mdboom Nov 6, 2014

Contributor

The {} need a number for Python 2.6 compatibility.

This comment has been minimized.

Copy link
@mdboom

mdboom Nov 6, 2014

Contributor

Try to keep lines to 100 columns or less.

@rhewett

This comment has been minimized.

Copy link

rhewett commented Nov 6, 2014

So as the one who suggested a decorator for this, I think I'd still prefer to just use exceptions to catch actions on quantities that don't behave like quantities (to take advantage of duck typing and all that).

That said, what I had in mind was a slightly different idea that does not require specifying every argument's type. Instead, the decorator handles arbitrary entries (and even **kwargs). Perhaps something like the following (note, it needs some love to handle default arguments and things like that):

import inspect
import numpy as np


class validate_type(object):

    def __init__(self, *args):

        self.pairs = zip(args[::2], args[1::2])

    def __call__(self, wrapped_func):

        def wrapper(*func_args, **func_kwargs):

            args, varargs, keywords, defaults = inspect.getargspec(wrapped_func)

            for name, target_type in self.pairs:

                if name in args:
                    loc = args.index(name)

                    if (loc < len(func_args)) and not isinstance(func_args[loc], target_type):
                        raise TypeError("Argument {0} of type {1} is not instance of type {2}".format(name, type(func_args[loc]), target_type))

                elif (name in func_kwargs) and not isinstance(func_kwargs[name], target_type):
                    raise TypeError("Argument {0} of type {1} is not instance of type {2}".format(name, type(func_kwargs[name]), target_type))

            wrapped_func(*func_args, **func_kwargs)

        return wrapper


class BarType(object):
    pass

A = BarType()
B = BarType()
C = BarType()


@validate_type("a", BarType, "c", BarType, "e", BarType)
def all_cases(a, b, c=None, d=None, **kwargs):

    pass


@validate_type("first", int, "second", np.ndarray)
def other_test(first, second):
    pass

# Succeeds
other_test(4, np.random.rand(5))

# Succeeds
all_cases(A, 2, c=B, d=10, e=C, f=12)

# Fails
all_cases(1, 2, 3)
@rhewett

This comment has been minimized.

Copy link

rhewett commented Nov 6, 2014

I should add that the above uses the inspect module, which occasionally feels like cheating, but it does the trick in this case.

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Nov 6, 2014

@rhewett I am a little confused, is your example just checking type?

My version catches two scenarios, one where the object doesn't have a .to attribute and one where that method raises a astropy.units.UnitsError which is the only way I can see to validate a incorrect unit. This means that in my example you should be able to feed in any object that has a .to attribute and raises a UnitsError if the conversion is invalid.

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Nov 6, 2014

I should also point out that I have now removed the conversion, it seems illogical to have it there, it will now pass through the object in the same units as it was passed in as long as it can be converted to the expected unit, and therefore any arithmetic will work.

@rhewett

This comment has been minimized.

Copy link

rhewett commented Nov 6, 2014

@Cadair, you can modify mine to do exactly that. The point is more that you don't have to specify behavior for each argument, only the ones that you want.

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Nov 6, 2014

The way I tend to handle scenarios like this is to use

Quantity(<input>, <desired-unit>)

If the input is something that has a unit attribute, this will be converted to the desired unit (raising UnitsError if this is not possible), and if the input does not have a unit, it is assumed that it has the correct unit. If the latter is undesired, one could check for a unit attribute before passing the value in to Quantity; I do think this is more robust than checking for the to method, and like that the testing is left to Quantity itself, thus separating concerns.

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Nov 6, 2014

@mhvk Something that was wrong with this in the first iteration is that we don't actually want to convert the input to the desired unit, we just want to know it is valid. For example, If a user is operating in km units we don't want the output to be in m. We would rather preserve the input unit where possible. Which means you need to do a try: except: block to verify this. That is the point of this decorator, to move that code out of every function.

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Nov 6, 2014

Separately, given PEP 3107 linked to by @mdboom above, I think it would be good to make the syntax as similar to that as possible. Note that there seems to be a python2.7 backport of sorts available, https://github.com/agoraplex/anodi.

@mhvk mhvk mentioned this pull request Nov 6, 2014
3 of 3 tasks complete
@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Nov 6, 2014

@Cadair - to check whether a unit is in principle convertible, you could either check its physical type, or you could use <input>.unit.is_equivalent(<to-be-checked>). The difference between the two is that the physical type check does not take into account equivalencies, while the is_equivalent check does. Thus, with the latter,

def your_code(angle):
    if not angle.unit.is_equivalent(u.radian):
        raise UnitsError("...")
    <do some work>

with u.add_enabled_equivalencies(u.dimensionless_angles()):
    your_code_that_requires_angles(input * u.one)

will work

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Nov 6, 2014

Or I guess

try:
    assert angle.unit.is_equivalent(u.radian)
except (AttributeError, UnitsError):
    raise UnitsError("Input is not a Quantity with angular units")

This way you also capture input that is not a quantity in the first place.

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Nov 6, 2014

@mhvk Your last example there only differs from my PR in the unit.is_equivalent call. I am not sure I see the advantage of unit.is_equivalent over to.

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Nov 6, 2014

@mhvk Having had a read of PEP 3107 I am not sure we can make the syntax similar. I am working on an updated version of this that uses kwargs for specifying units for both the functions args and kwargs like @rhewett's example.

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Nov 6, 2014

@Cadair - yes, we converged. The main advantage of is_equivalent is speed:

In [1]: import astropy.units as u, numpy as np
In [2]: q = np.arange(10.)*u.km
In [3]: %timeit q.unit.is_equivalent(u.m)
100000 loops, best of 3: 7.28 µs per loop
In [4]: %timeit q.to(u.m)
10000 loops, best of 3: 16.9 µs per loop

Since .to makes a copy of the data, the speed difference (and memory usage) will increase with size of the array.

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Nov 6, 2014

@mhvk cool I will use that instead.

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Nov 7, 2014

@Cadair - the example at the top you gave does seem like one where a function signature or annotation is more or less what one would want -- note also that @embray is suggesting we backport the signature stuff from python3.3 -- see #3084. I'd hope this could be general and very powerful but as I haven't had time to actually try it, it may all be wishful thinking... @embray - as you have been thinking about it for modelling, do you think the function signatures could be used to indicate that input should be a Quantity with a particular unit, as is the goal here?

@embray

This comment has been minimized.

Copy link
Member

embray commented Nov 7, 2014

Speaking of function annotations, there are backports of function annotations for Python 2. It's just that instead of using the annotation-specific syntax you just use a decorator instead which really is good enough (it will still add the __annotations__ attribute to the function just as though Python 3 annotations were used.

In principle you can even add support for the Python 3 annotation syntax in Python 2, but that's a much more epic hack (but yes, it is possible).

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Nov 8, 2014

@Cadair, I had a slightly longer look at the annotations, and it would work something like (EDIT - this is inspired by the discussion at http://stackoverflow.com/questions/3038033/what-are-good-uses-for-python3s-function-annotations -- better ways may be possible):

import astropy.units as u, numpy as np
def validate_units(function, locals_):
    for var, unit in function.__annotations__.items():
        if(isinstance(unit, u.UnitBase) and
           not (getattr(locals_[var], 'unit', u.dimensionless_unscaled)
                .is_equivalent(unit))):
            raise u.UnitsError("Parameter '{0}' with value '{1}' "
                               "does not have units convertible to '{2}'."
                               .format(var, locals_[var], unit))

# pure python3 example
def trial(value: u.radian):
    validate_units(trial, locals())
    print(value.to(u.arcsec))

# example that works with python2 and python3
def trial2(value):
    validate_units(trial2, locals())
    print(value.to(u.arcsec))

trial2.__annotations__ = {'value': u.radian}

trial2(1.*u.deg)
# 3600.0 arcsec
trial2(1.*u.m)
# ... UnitsError: Parameter 'value' with value '1.0 m' does not have units convertible to 'rad'.

So, the basics seem to be there, and I like the idea of being ready for python3 (quite a few of us have switched already).

A nice side-benefit is that in [i]python3, annotations are used in help:

help(trial2)
# Help on function trial2 in module __main__:
# 
# trial2(value:Unit("rad"))

trial2?
# Type:        function
# String form: <function trial2 at 0x7f4204b8cea0>
# File:        /home/mhvk/<ipython-input-11-45c8b96c5580>
# Definition:  trial2(value: Unit("rad"))
# Docstring:   <no docstring>
@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Nov 8, 2014

p.s. The above does not necessarily mean we should not have a decorator! Rather, I think it would be good to use __annotations__ internally. Also, it would likely be easiest if the decorator took a dict keyed on argument names (i.e., something that could directly be stored in __annotations__; the decorator would then also include the validate_units call). Oviously, we'd also want to keep the signature of the function intact, so #3084 would still be relevant.

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Nov 8, 2014

All the above said, one thing to reiterate is that this type/unit-checking is costly -- in my opinion, in most cases it is better to just let the code fail where it becomes important that the input has a given unit.... Indeed, one could imagine wrapping the whole function in a try/except, and only if something goes wrong check whether an argument had the wrong units, to make the exception more helpful.

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Nov 21, 2014

I haven't had a chance to update the tests yet, but the API is now like this:

Python 3:

@QuantityInput()
def trial(value: u.m, kwvalue: u.m=100*u.m):
    print(value.to(u.arcsec))

trial(100*u.deg)

Python 2:

@QuantityInput(value=u.m, kwvalue=u.m)
def trial(value, kwvalue=100*u.m):
    print(value.to(u.arcsec))

trial(100*u.deg)
@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Nov 21, 2014

ping @mhvk

Cadair and others added 11 commits Nov 27, 2014
[ci skip]
…r should write the entire test in the test's docstring, so that it may contain Python 3 only syntax. On Python 2 the test will be marked skip. On Python 3 the test body will by compiled from the docstring and executed. Perhaps some version of this could be moved to general test utilities, but for now this is the only test module we have, I think, that could make use of this.
@Cadair Cadair force-pushed the Cadair:quantity_decorator branch from 27fb028 to 9ca243e Dec 16, 2014
@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Dec 16, 2014

@astrofrog rebased.

@astrofrog

This comment has been minimized.

Copy link
Member

astrofrog commented Dec 16, 2014

This looks good to me, so I'll merge by tonight if there are no objections.

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Dec 16, 2014

:D

@Cadair Cadair force-pushed the Cadair:quantity_decorator branch from 4b447f7 to 50a89ca Dec 17, 2014
@embray

This comment has been minimized.

Copy link
Member

embray commented Dec 17, 2014

👍 especially if it is helpful for SunPy.

CHANGES.rst Outdated
@@ -191,6 +191,9 @@ New Features
- Add ability to use ``WCS`` object to define projections in Matplotlib,
using the ``WCSAxes`` package. [#3183]

- Added units.quantity_input decorator to validate quantity inputs to a
function for unit compatibility. [#3072]

This comment has been minimized.

Copy link
@embray

embray Dec 17, 2014

Member

D'oh--just realized something went awry here. This should be moved above, to the section for astropy.units (each changelog section has subsections for each Astropy subpackage).

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Dec 17, 2014

@embray I moved the changelog entry, forgot to [ci skip] it though.

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Dec 17, 2014

@Cadair - I killed that last build so we don't waste travis kWh (and delay other astropy commits). Since @astrofrog had already indicated he would merge yesterday, I go ahead and merge now.

mhvk added a commit that referenced this pull request Dec 17, 2014
Add a new decorator for functions accepting quantities.
@mhvk mhvk merged commit 0172e6f into astropy:master Dec 17, 2014
1 check was pending
1 check was pending
continuous-integration/travis-ci The Travis CI build is in progress
Details
@embray

This comment has been minimized.

Copy link
Member

embray commented Dec 17, 2014

I generally like it when people rebase after minor changes like that too, to cut down on noise in the history. But it's not worth going back and fussing with at this point.

@mhvk

This comment has been minimized.

Copy link
Contributor

mhvk commented Dec 17, 2014

Agreed, though we should also balance that with not making the process too painful. That said, maybe we can update the developer pages to include this (if it isn't there already, I haven't looked for ages...).

@Cadair

This comment has been minimized.

Copy link
Member Author

Cadair commented Dec 18, 2014

Woooyeah! Thanks for your help everyone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.