Porting the converting decorators from v2

[snilsn]

I have ported the 4 converting decorators that enable a rudimentary xarray support from tobac v2 to 1.5. I also added some tests and fixed some bugs I found during this process.

• Have you followed our guidelines in CONTRIBUTING.md?
• Have you self-reviewed your code and corrected any misspellings?
• Have you written documentation that is easy to understand?
• Have you written descriptive commit messages?
• Have you added NumPy docstrings for newly added functions?
• Have you formatted your code using black?
• If you have introduced a new functionality, have you added adequate unit tests?
• Have all tests passed in your local clone?
• Have you kept your pull request small and limited so that it is easy to review?
• Have the newest changes from this branch been merged?

Not yet, probably not needed

• If you have introduced a new functionality, have you added an example notebook?

[codecov]

Codecov Report

Base: 39.03% // Head: 40.82% // Increases project coverage by +1.78% 🎉

Coverage data is based on head (9f83209) compared to base (7e961e6).
Patch coverage: 98.57% of modified lines in pull request are covered.

Additional details and impacted files

@@              Coverage Diff              @@
##           RC_v1.5.0     #179      +/-   ##
=============================================
+ Coverage      39.03%   40.82%   +1.78%     
=============================================
  Files             11       11              
  Lines           2262     2332      +70     
=============================================
+ Hits             883      952      +69     
- Misses          1379     1380       +1     

Flag	Coverage Δ
unittests	40.82% <98.57%> (+1.78%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
tobac/utils.py	58.85% <98.57%> (+10.56%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[freemansw1]

Thanks for doing this, @snilsn . My initial review is to approve these without changes, but I do want to take the time to check what happens if we port a tobac function over using these. I do wonder if we want to automatically convert output to xarray? It would be nice in v1.5 or v1.6 to let things be an option (i.e., allow users to output as pandas while giving an xarray). That said, I think that's a different PR, and this one from v2.0-dev is important.

Given that you were here first, I'm going to recommend merging this before #191, and I will update #191 to match. These functions are in utils/convert.py, so I will put them back in there.

[snilsn]

Providing different options is a good idea @freemansw1. Partially this option with putting in xarray, but getting out pandas is already implemented with the diferent decorators, xarray_to_iris will not transform dataframe outputs to xarray, while xarray_to_irispandas will.

The last test I designed as an integration test, using one of the decorators to port the main tobac functions, and then checking the output vs the output of the undecorated functions. Seems to work at least for a test dataset.

[freemansw1]

Good point, @snilsn. And implementing these won't impact the user-facing code for now anyway, so I've approved the PR.

[snilsn]

I worked on a more streamlined testing approach for this, systematically checking a lot of input combinations. I found one bug, where the keys of a dict were accessed instead of the values. Fixed that for all decorators.

More importantly, there is some inconsistent behavior:

If a function is decorated to support a certain datatype, but the "old" datatype is used as input, there are cases where the output is converted to the new datatype, and others where it isn't. It depends on the presence of keyword arguments that are converted.

I don't think we need to address this right now, since the conversion of the output should probably be optional as @freemansw1 said, but worth noting.

[freemansw1]

Good points, and good change @snilsn ! I expect that we may modify these functions in 1.6+ as we start getting everything integrated, but good to start checking/integrating these now. I'm still happy with the changes. We can merge once @JuliaKukulies has time to review.

[kelcyno]

@snilsn @freemansw1 I tested the decorators using radar data and all seemed to flow as necessary. There is the example of use cases in the test file, but it would be useful/helpful to add an example in the utils.py file for the function. Something like:
Example usage:
get_spacings_xarray = tobac.utils.xarray_to_iris(tobac.utils.get_spacings)
dxy, dt = tobac.utils.get_spacings(data)
dxy_xarray, dt_xarray = get_spacings_xarray(data_xarray)

However, I realize this isn't standard in the utils.py and I'll defer to @freemansw1 opinion here.

[freemansw1]

Thanks very much for reviewing, @kelcyno! I think an example use would be helpful in the docs, but I would guess that the primary use would be as a decorator, so we would @xarray_to_iris above the get_spacings function in the code. I'm hesitant to start recommending this to users (especially given the headaches that one can encounter between xarray and iris), but I'm certainly happy if more technical users want to do so before we start decorating the functions.

All that said, yes, I agree that we should put the basic use in the docs. Great suggestion!

[snilsn]

Totally see your point @kelcyno, I'm going to add something like this.

Just wasn't sure if we are going to threat these decorators as purely internal utilities or not, but i guess it's also a good idea for testing and experimenting with them.

[kelcyno]

If you're fine leaving them as internal functions, we can think about examples for other use cases in the future releases and document them as needed..

[freemansw1]

I am inclined for @snilsn to add the documentation as you suggested, @kelcyno.

Also, with the merger of v1.4.0 back into this branch, looks like we created a conflict. I'll wait to merge until the conflict has been resolved and the docs have been added.

[snilsn]

@freemansw1 @kelcyno I resolved the conflict and added examples for xarray_to_iris and xarray_to_irispandas. I think the other two decorators don't have a use case atm, since I'm not aware of a function that uses xarray internally. Is there any?

For the examples I followed the numpydocs styleguide and the result with the highlighted codesnippets looks actually quite nice:

https://tobac--179.org.readthedocs.build/en/179/tobac.html#module-tobac.utils

[JuliaKukulies]

Nice, great work @snilsn ! Now I also got the time to test your decorators and I have nothing to add. Just a small comment on the documentation, otherwise it looks all good!

[freemansw1]

I'm happy with the changes, so as long as @kelcyno approves your most recent changes, we can merge.

[kelcyno]

@freemansw1 @snilsn I'm happy with the changes that have been made, glad to approve.

[freemansw1]

Fantastic! @snilsn are you happy to merge? If so, I'll merge.

[snilsn]

Yes, please go ahead @freemansw1