-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Refactor quantity_input
#10655
Refactor quantity_input
#10655
Conversation
Hello @nstarman 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style. There are no PEP8 style issues with this pull request - thanks! 🎉 Comment last updated at 2022-01-03 11:56:11 UTC |
I like very much where this is going! But as with other rather big PRs, I worry this is of a size where it will be difficult (at least for me) to find time to properly review... (I'm supposed to work on resubmission of a paper right now!). Of course, I want to be sure not to cause too much work either, but might it be an idea to try to break this down into a couple of steps, which we can judge separately (and ideally each are useful)? E.g.,
An advantage of this order is that we go first towards enhancing What do you think? I hope that the above would not make much more work... If we go this route, it might be easiest to open a new PR, and leave this one to give an overall idea of where this is going (your description on top is very helpful!). p.s. Final friendly request: if possible, try to minimize code style changes; it makes review easier. |
Thanks for the detailed response. I agree that this PR might be best split into a few pieces. It kind of ballooned as I was writing the description. While I have much of the code written, as they say: the first 80% takes only 20% of the time. Getting that last 20% will be time consuming. I've been thinking a while about the paradigm of coordinating everything through annotated typing and if there were an elegant alternative. Since we have the essential long-term goal of allowing static typing, I do not think there is any reasonable alternative. Therefore, I think the best incremental approach is to mimic much of the static typing approach, without actually producing or using static typing. Specifically, instead of using the My proposed series of PRs is:
I think a good fix to the question of I would suggest, though this is optional, to add another method to
How are these types of multiple PRs generally coordinated? WIth regards to the necessity of UnitSpec, no it is not strictly necessary. However, it drastically simplifies a few things. For instance, in if isAnnotated(antn):
md = [isinstance(x, UnitSpecBase) for x in antn.__metadata__]
if not any(md): # skip to next argument
continue
elif sum(md) > 1 : # more than one UnitSpec
raise Exception()
unitspec = antn.__metadata__[np.nonzero(md)]
if name == "return": # save the return casting until actually call function.
return_spec = unitspec
else:
ba.arguments[name] = unitspec(ba.arguments[name]) Also, to subclass For example, to make the class DeQuantityInput(QuantityInput):
"""QuantityInput, with ``to_value(unit)`` on all specified inputs."""
def __call__(self, wrapped_function):
# use all the machinery from superclass
wrapper = super().__call__(wrapped_function)
# but now need to ensure that everything is correct UnitSpec
_antns_ = dict()
for name, antn in wrapper.__annotations__.items():
if isAnnotated(antn):
unitspec = antn.__metadata__[0]
if isinstance(unitspec, UnitSpecBase):
if name == "return":
_antns_[name] = UnitSpec(unitspec, action="from_value")
else:
_antns_[name] = UnitSpecValue(unitspec)
wrapper.__annotations__.update(_antns_)
return wrapper Furthermore, argument-specific behavior in |
@nstarman - excellent summary, it looks like we are on the same page for the schedule. The arguments for |
PR #10662 might be delayed to Astropy 5.0, when py3.6 EOLs. |
Well, the delay is up for discussions. 😬 I am just thinking that if we can avoid much custom code if we wait for bump to Python 3.7, there is no reason not to wait? Anyways, other ideas welcome! |
I completely agree about avoiding a metaclass for Quantity just to accommodate Python 3.6. It's especially not a problem as the proposed |
This looks awesome, I will try and find enough time to really dive into it soon 🚀 |
Hello, given the draft status of this PR and the impending feature freeze tomorrow, I am moving the milestone. Thank you for your understanding. |
No problem. I’ll move the info to the next version. Thanks for the notice.
… On Oct 22, 2020, at 15:32, P. L. Lim ***@***.***> wrote:
Hello, given the draft status of this PR and the impending feature freeze tomorrow, I am moving the milestone. Thank you for your understanding.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#10655 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACEI7EOF74OLAJBECWKC2S3SMCCGNANCNFSM4P7D5BKQ>.
|
Thanks for your patience, @nstarman ! Please hold off on moving the change log until new section exists. 😄 |
@pllim, plz push to next milestone :) |
93180c5
to
8c36fe0
Compare
It's that time again @nstarman :) -- any sense of what the next steps here would be? |
8c36fe0
to
b93bb3d
Compare
8762285
to
46e8fd1
Compare
46e8fd1
to
e3e5274
Compare
Hi humans 👋 - this pull request hasn't had any new commits for approximately 4 months. I plan to close this in 30 days if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary. If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
Sigh. I have a working implementation, but I'm unhappy with the large code burden it adds. |
I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks! If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here. |
Description
This is a big set of changes, PLEASE HELP.
This DRAFT pull request is to coordinate a number of PRs improving
quantity_input
and unit-aware static typing:The goals are:
quantity_input
with statically typed codeSpecificTypeQuantity
subclasses, like Distance or Angle, to be used instead ofQuantity
in the input verification ofquantity_input
quantity_input
-wrapped functions by pre-computing as much of the conversion mechanics at wrapper initialization.b) post-cast, to pass input to function as
.to_value()
so that functions can work with ndarrays guaranteed to be in the correct unitsSelect Ancillary Benefits:
See whole comment for details
quantity_input
UnitSpec
classes can be used independently ofquantity_input
to provide specific unit/physical-type validation/conversion/etc.UnitSpec
subclasses are automatically registered and usable byUnitSpec
andquantity_input
Summary of each PR
Annotated[Quantity, <unit>]
as official unit-aware Quantity annotation__class_getitem__
method toQuantity
to shortcut creating.Caveat: py3.7+
quantity_input
Rewrite
quantity_input
internals so that all information is stored inwrapper.__annotations__
, converting valid inputs toQuantity[<unit or pt>]
from PR#1. This is still not valid static typing, but it will trivially solve Allow return annotations other than Quantity #10156 by having a) non-unit / physicaltype return annotations ignored and b) allowingquantity_input(return=None)
to store None inwrapper.__annotations__
instead ofQuantity[<unit or pt>]
UnitSpec
to solve Decorator for converting quantity to arrays in specific units? #7368 and multiple annotationsOverall Summary
Subject to change.
Static Typing
Current State:
There is no means to create unit-aware type annotations. For instance, the annotations used by
quantity_input
, likeu.km
ordistance
, are not compliant with static typing since the inputs are of (sub)typeQuantity
.Proposed Change:
This problem can be solved by the
typing.Annotated
class, from this PEP which allows valid static type annotations to include metadata. Now, unit-aware type annotations will be of the formAnnotated[Quantity, <unit>, other_annotations]
, where the unit must be the first annotation.A
__class_getitem__
method will be added toQuantity
to shortcut creating unit aware annotations.Quantity[<unit>, other_annotations] == Annotated[Quantity, <unit>, other_annotation]
.As a class method, this will similarly work for all subclasses of
Quantity
.quantity_input
will accept unit-aware quantity annotations of this type. Both the type (eg Quantity) and unit must be valid inputs.Implementation Details:
typing_extensions is the official backport for Annotated and works on any py3.5+. Until py3.9+ is the minimum python version, when
Annotated
is in the built-intyping
library, we use thetyping_extension
backport. Until thetyping_extension
backport is added as an astropy dependency we emulate API for unsupported features.Pre-Computation and User Accessibility
Current State:
quantity_input
uses function annotations to determine the required argument units and since it also usesfunctools.wraps
those annotations are propagated to the wrapper returned byquantity_input
. However, other inputs toquantity_input
, such as keyword arguments are not similarly stored.Proposed Change:
The wrapper's
__annotations__
will store all unit/physical_type information, merging the information provided from the keyword arguments with the original function's__annotations__
. The original function will be unchanged and accessible in the__wrapped__
attribute. Since decorators can be applied as functions, not just with the "pie" syntax, this will allow multiple different versions of the same function to be created with different unit specifications. Not only that, but storing all information in__annotations__
means that the desired units are pre-computed, increasing function call speed, and are also editable by knowledgeable users in a python-standard format.UnitSpec
For
quantity_input
, theAnnotated[Quantity, <unit>, other_annotations]
is not specific enough, since it only annotates which argument should be processed, not what to actually do with them. We introduce aUnitSpec
(abbreviation for "unit specification") class to do both.quantity_input
will accept the standardAnnotated[Quantity, <unit>, other_annotations]
, but also accept the more detailed annotationAnnotated[Quantity, UnitSpec(<unit>), other_annotations]
. Internally, any of non-unitspec annotations will be converted.The
UnitSpec.__call__
will control howquantity_input
interacts with each input — if it just verifies unit / physical_type, or casts to unit / physical_type, or doesto_value
, etc. UnitSpec will enable different treatment of each input: for a functionfunc(x, y)
,x
might just be verified, buty = y.to_value(u.km)
.UnitSpec
will have the following pre-specified options for the__call__
output, set with the "action" attribute..to_value
after conversionThe following subclasses of
UnitSpec
will also be implemented:UnitSpecValidate
: which can only validate physical typeUnitSpecConvert
: which will convert quantities to specific units / physical typeUnitSpecValue
: a subclass ofUnitSpecConvert
which takes.to_value
after conversionUnitSpecAssign
: which reassigns units if none present & verifies units if present.Using
__init_subclass__
these and any user-created subclasses ofUnitSpec
will be added to a registry. The "action" kwarg ofUnitSpec
must be a key in this registry and will be used byUnitSpec
to set the behavior ofUnitSpec.__call__
. In this wayUnitSpec
can stand in for any of its subclasses, which are only really needed to make a behavior immutable.UnitSpec
will drastically simplify a few things inquantity_input
, and potentially elsewhere.For instance, in
quantity_input
, all that's needed to check a multiply-annotated argument and then perform the validation isquantity_input
implementationOrder of precedence of unit specifications:
quantity_input
:quantity_input(x=u.hour)(foo)
.This allows
quantity_input
to wrap functions from external libraries without modifying the function.So that calling
quantity_input
as a function overrides the default unit specifications.To respect type hints
Argument-specific behavior in
quantity_input
is allowed by mixingUnitSpec
subclasses inwrapper.__annotations__
.For simple means of ensuring uniform
UnitSpec
behavior, there will be a few subclasses ofQuantityInput
which convert any user-created unit-aware Quantity annotations to the appropriateUnitSpec
subclasses. The complexities of thewrapper
construction can happen insuper
and only ever need to be written once. TheUnitSpec
auto-registry very easily allows user-createdUnitSpec
and therefore user-createdQuantityInput
subclasses.ValidateQuantityInput
: convertsUnitSpec
toUnitSpecValidate
ConvertQuantityInput
: convertsUnitSpec
toUnitSpecConvert
DeQuantityInput
: convertsUnitSpec
toUnitSpecValue
For example, to make a
to_value
flavor ofquantity_input
is a simple for-loop replacingUnitSpec
withUnitSpecValue
, the subclass that performs conversion and takes the value. While this is the default behavior forUnitSpec
and the subclass used byUnitSpec(...action = 'value')
, the subclass is immutable and more explicit.Examples
Assuming the following imports
Example 1)
To force
x
input to be Distance or subclassTo change
t
to action "convert"To make a
to_value
flavor ofquantity_input
is a simple for-loop replacingUnitSpec
withUnitSpecValue
, the subclass that performs conversion and takes the value. While this is the default behavior forUnitSpec
and the subclass used byUnitSpec(...action = 'value')
, the subclass is immutable and more explicit.Fixes #10156, #5606, #7368