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

expects decorator #143

Open
wants to merge 55 commits into
base: main
Choose a base branch
from

Conversation

TomNicholas
Copy link
Member

@TomNicholas TomNicholas commented Oct 20, 2021

WIP attempt to add an @expects decorator to the API, which emulates pint.UnitRegistry().wraps.

  • Closes @ureg.wraps decorator #141
  • Tests added
  • Passes pre-commit run --all-files
  • User visible changes (including notable bug fixes) are documented in whats-new.rst
  • New functions/methods are listed in api.rst

@TomNicholas TomNicholas added the enhancement New feature or request label Oct 20, 2021
@TomNicholas TomNicholas marked this pull request as draft October 20, 2021 23:13
Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

If I understand wraps correctly, it makes sure that the input data is in the specified units, strips them and attaches the units specified as return value to the results. Should we copy that design for expects?

Edit: It would also make sense to have it stay the way it currently is, though

pint_xarray/checking.py Outdated Show resolved Hide resolved
pint_xarray/checking.py Outdated Show resolved Hide resolved
pint_xarray/checking.py Outdated Show resolved Hide resolved
@TomNicholas
Copy link
Member Author

If I understand wraps correctly, it makes sure that the input data is in the specified units, strips them and attaches the units specified as return value to the results. Should we copy that design for expects?

Edit: It would also make sense to have it stay the way it currently is, though

I actually did not realise that. I just kind of started implementing it the way I felt it should work. That is not that different though - perhaps we could have both behaviours via an extra kwarg pass_magnitude or something?

@keewis
Copy link
Collaborator

keewis commented Oct 23, 2021

I actually did not realise that. I just kind of started implementing it the way I felt it should work. That is not that different though - perhaps we could have both behaviours via an extra kwarg pass_magnitude or something?

the disadvantage of a new kwarg is that it could shadow a variable (if we choose the name carefully enough that might not be as much of an issue, though). What do you think about adding a new decorator that does both? It could share all internals and additionally dequantifies before and quantifies after the call to the user function.

@TomNicholas
Copy link
Member Author

the disadvantage of a new kwarg is that it could shadow a variable (if we choose the name carefully enough that might not be as much of an issue, though)

Any xarray function that only accepts kwargs but also has its own kwargs has this problem - we could copy xarray by allowing users to pass a dict of kwarg_units optionally in the same way that .sel does?

What do you think about adding a new decorator that does both?

Or we could just do that. It just needs a good name. I still think @wraps is a poor name, how about @expects_magnitude? (Because the wrapped function "expects" to be given a magnitude)

@TomNicholas
Copy link
Member Author

What do you think about adding a new decorator that does both? It could share all internals

@keewis I don't know if I'm being dense but I actually can't work out how to make two differently-behaving decorators that share most of their internals 😅 I even made a SO question about it, but no-one has answered it yet. I could just copy-paste the internals code for now...

@keewis
Copy link
Collaborator

keewis commented Oct 28, 2021

if you look at the structure of a decorator without arguments, it's usually something like:

def decorator(func):
    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        # preprocess args / kwargs
        result = func(*args, **kwargs)
        # postprocess the result
        return result
    return wrapper

if you put the pre/postprocessing into separate functions, the new decorator can call those and do some additional preprocessing to drop the units (and add them back in the postprocessing step)

@TomNicholas
Copy link
Member Author

if you put the pre/postprocessing into separate functions, the new decorator can call those

Yes, but I think you still need to duplicate all the triple wrapping code, like this

def decorator1(func):
    flag = True
    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        preprocess(args, kwargs, flag)
        result = func(*args, **kwargs)
        postprocess(result, flag)
        return result
    return wrapper


def decorator2(func):
    flag = False
    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        preprocess(args, kwargs, flag)
        result = func(*args, **kwargs)
        postprocess(result, flag)
        return result
    return wrapper

because if you instead try to move the def wrapper out separately, it no longer has access to func...

@keewis
Copy link
Collaborator

keewis commented Oct 28, 2021

wouldn't it be possible to do this instead:

def preprocess1(*args, **kwargs):
    ...
    return args, kwargs

def preprocess2(*args, **kwargs):
    ...
    return args, kwargs

def decorator1(func):
    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        args, kwargs = preprocess1(*args, **kwargs)
        result = func(*args, **kwargs)
        result = postprocess(result)
        return result
    return wrapper

def decorator2(func):
    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        args, kwargs = preprocess1(*args, **kwargs)
        args, kwargs = preprocess2(*args, **kwargs)
        result = func(*args, **kwargs)
        result = postprocess(result)
        return result
    return wrapper

that's probably not as elegant as using a flag, but maybe it's simpler?

@TomNicholas
Copy link
Member Author

wouldn't it be possible to do this instead:

Yeah, but that doesn't really solve my question - it still duplicates def wrapper (and for a decorator with arguments there is another layer of wrapper that would get duplicated too).

It might be possible to solve this by making a callable class that binds all the arguments as attributes, but it's probably not worth the effort.

@keewis
Copy link
Collaborator

keewis commented Oct 28, 2021

fair point, I didn't consider the outer layer yet. I'll try to come up with something that also deduplicates that (might take some time, though), but in the meantime it might be fine to only share pre/postprocessing.

@TomNicholas
Copy link
Member Author

If I understand wraps correctly, it makes sure that the input data is in the specified units, strips them and attaches the units specified as return value to the results. Should we copy that design for expects?

Thinking about this again I think you might be right. To be useful, an @expects decorator needs to deliver a magnitude to the internal function, because what is the use case of supplying a quantified pint array with a specific unit?

In other words: when would I ever have a function that (a) specifically wants pint Quantities instead of bare numpy(/xarray) objects but also (b) will only accept those quantities with a specific unit?

@keewis
Copy link
Collaborator

keewis commented Nov 27, 2021

I agree, it looks like just verifying the units' dimensions is not that useful (and this can also be achieved in the function using either is_compatible_with or just using the data and having the errors bubble up).

@TomNicholas
Copy link
Member Author

@keewis I think this might be ready to merge now finally??

Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

apologies for letting this sit for almost two weeks again.

I agree, this is fairly close. I still have a few comments regarding the docs, but one of them might also affect the code.

pint_xarray/checking.py Outdated Show resolved Hide resolved
pint_xarray/checking.py Show resolved Hide resolved
pint_xarray/checking.py Show resolved Hide resolved
pint_xarray/checking.py Show resolved Hide resolved
Comment on lines +77 to +81
If an argument or return value has a specified unit, but is
not an xarray.DataArray or pint.Quantity. Also thrown if any
of the units are not a valid type, or if the number of
arguments or return values does not match the number of units
specified.
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't we raise ValueError if the number of arguments / return values do not match?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah possibly. I can change that.

@keewis
Copy link
Collaborator

keewis commented Jan 31, 2022

good news is that the next release of pint shouldn't be too far off (see hgrecco/pint#1450), so that might help with the blog post.

pint_xarray/checking.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@keewis keewis left a comment

Choose a reason for hiding this comment

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

as mentioned in https://github.com/xarray-contrib/pint-xarray/pull/143/files#r815333587, Signature.bind is really useful for this.

I've added a few suggestions that should hopefully make this much easier, but it requires to also specify units for keyword-only parameters, and *args and **kwargs are still not supported.

Edit: We'd probably need quite a few tests, too

pint_xarray/checking.py Outdated Show resolved Hide resolved
pint_xarray/checking.py Outdated Show resolved Hide resolved
@keewis
Copy link
Collaborator

keewis commented Sep 21, 2022

I've implemented my suggestion to use Signature.bind_partial / Signature.bind to normalize the parameters, so I think this leaves us with just a few open issues:

  • we ignore / disallow units in *args and **kwargs for now because checking a variable number of arguments seems way too complicated (except if all should have the same units, but that exception might not be worth considering).
  • I couldn't find a nice error message for the case where some parameters of the decorated function don't have units
  • specifying None as the unit will cause any units on objects passed to that parameter to be ignored. Do we want that, or should we raise an error for quantities passed to those parameters? Edit: what if we introduce checking for None and ignore for ... / Ellipsis?
  • The documentation is pretty minimal at the moment. Not sure whether a more elaborate user guide (I think we can use the apply_ufunc guide as a reference for that) should be added in this PR, or whether we should merge and try how it does in practice, and only then write that guide?
  • the units of the return value will currently always use the application registry. We should try to use the current registry instead, most likely by normalizing the input and output units before the conversion (by converting all units to either None or Unit objects)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@ureg.wraps decorator
2 participants