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

Accept non-unit type annotations in @quantity_input #7672

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ritiek
Copy link
Contributor

commented Jul 21, 2018

Closes #7661.

Instead of raising an error, @quantity_input will skip any non-unit type annotations in function parameters. So, code such as below won't result in an error:

>>> import astropy.units as u

>>> @u.quantity_input
... def f(x: u.m, y: str) -> u.m:
...    return x

>>> f(1 * u.m, 'x')
<Quantity 1. m>
# this would otherwise give
# TypeError: <class 'str'> can not be converted to a Unit

It will however, raise a ValueError in cases like these:

>>> import astropy.units as u

>>> @u.quantity_input
... # consider the typo "kilogrammm" instead of "kilogram"
... def f(x: u.m, y: "kilogrammm") -> u.m:
...    return x

>>> f(1 * u.m, 2 * u.kg)
ValueError: Invalid unit or physical type 'kilogrammm'.

TLDR; @quantity_input now ignores any parameter in function definition which is non-unit type annotated, but will raise a ValueError if a string is passed and Unit(string) is unable to parse it (like above example).

@astropy-bot

This comment has been minimized.

Copy link

commented Jul 21, 2018

Hi there @ritiek 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

I noticed the following issue with this pull request:

  • Changelog entry section (v3.1) inconsistent with milestone (v3.2)

Would it be possible to fix this? Thanks!

If there are any issues with this message, please report them here.

@ritiek

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2018

cc @namurphy.

@bsipocz bsipocz added the units label Jul 21, 2018

@bsipocz bsipocz added this to the v3.1 milestone Jul 21, 2018

is_equivalent = arg.unit.is_equivalent(allowed_unit,
equivalencies=equivalencies)
# any other annotation was provided except for ``astropy.unit``
if inspect.isclass(allowed_unit):

This comment has been minimized.

Copy link
@Cadair

Cadair Jul 21, 2018

Member

Can you explain this to me? I don't understand how it helps to filter out Unit classes.

This comment has been minimized.

Copy link
@ritiek

ritiek Jul 21, 2018

Author Contributor

With all respect, I'm not sure if this is the best way to check if the annotation is a unit or non-unit.

Unit("kg"), u.kg, u.m, etc. are not classes, so this returns False. While str, int, etc. are classes, so this returns True in those cases enabling us to differentiate between unit and non-unit annotations.

I don't understand how it helps to filter out Unit classes.

I don't know if @quantity_input works with annotation as Unit class too as it does with Unit objects. Is this the case?

@@ -23,8 +23,10 @@ def _get_allowed_units(targets):
try: # unit passed in as a string
target_unit = Unit(target)

except ValueError:
except TypeError: # unable to convert to target_unit
target_unit = target

This comment has been minimized.

Copy link
@mhvk

mhvk Jul 21, 2018

Contributor

I think this is strange - if one cannot interpret the target as a unit, I think one should not still append it to the allowed units, ie., I would just do continue here. I would similarly check below whether target is string-like (which is needed for mapping to physical type), and just continue if it is not.

I guess the idea would be that this routine is the one that checks the annotations: anything that Unit accepts is OK, as is any string (with those not a unit or physical type being the only ones that raise an error), anything else is ignored.
With that idea, where this function is called below, you would just omit any check if allowed_units is empty.

This comment has been minimized.

Copy link
@mhvk

mhvk Jul 21, 2018

Contributor

p.s. Maybe didn't quite think this through properly: obviously ignoring non-unit items should only be done if one is looking at annotations, not for direct input to the function,

@bsipocz bsipocz modified the milestones: v3.1, v3.2 Oct 27, 2018

@bsipocz

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@ritiek - any plans to address the review in the coming 2 days to get this into the new release, or you prefer to get this remilestoned to 4.0?

@ritiek

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2019

@bsipocz My apologies, I completely forgot about this PR.

I'm currently in the midst of my final exams and probably won't have time to work on this until this weekend. It would probably be best if you guys could re-milestone as I wouldn't want the release to delay cuz of this PR.

@bsipocz

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@ritiek - thanks for getting back, and good luck for the exams.

@bsipocz bsipocz modified the milestones: v3.2, v4.0 Apr 18, 2019

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