-
Notifications
You must be signed in to change notification settings - Fork 54
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 ability to save whether or not an iris
to xarray
conversion happened in the decorators
#380
Add ability to save whether or not an iris
to xarray
conversion happened in the decorators
#380
Conversation
iris
to xarray
conversion happenediris
to xarray
conversion happened in the decorators
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## RC_v1.5.x #380 +/- ##
=============================================
+ Coverage 56.91% 57.05% +0.13%
=============================================
Files 20 20
Lines 3440 3472 +32
=============================================
+ Hits 1958 1981 +23
- Misses 1482 1491 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A function that creates a function that contains a wrapper... python decorators are a little messy... A couple of minor suggestions, but I'm happy for this to be merged
tobac/utils/decorators.py
Outdated
else: | ||
output = func(*args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines should be removed, as they are recalculating the output when it could just be returned as is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand how to resolve this. I tried to delete the two lines, but you then reference output
before assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's odd, as output should be defined on line 150. Did you remove these two lines (163-164) or the similar lines at 166-167 (which are still required)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I had deleted the (wrong) similar lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a comment about the docstrings, otherwise happy to merge.
Linting results by Pylint:Your code has been rated at 8.72/10 (previous run: 8.72/10, +0.00) |
@w-k-jones @snilsn I am ready for re-review when you are! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates, breaking out the conversion process into separate functions is a good idea. Only one comment regarding lines 163-164 but otherwise happy to merge
tobac/utils/decorators.py
Outdated
else: | ||
output = func(*args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's odd, as output should be defined on line 150. Did you remove these two lines (163-164) or the similar lines at 166-167 (which are still required)
Ok, resolved that last comment. @w-k-jones, @snilsn ready to merge when you are. |
Great! Happy for this to be merged |
Great, please go ahead and merge! @freemansw1 |
Pulls code from #354 to allow developers to specify (with a new argument to the decorator
irispandas_to_xarray
) whether or not aniris
->xarray
conversion occurred, via a passed keyword argument.Important note: I've now changed all of our decorators to allow keyword arguments. That means that when calling decorators in the future, you must call with an empty
()
regardless of whether or not you want to use them.