Adding ability to track on one dataset, segment on another

[freemansw1]

This is new functionality that I first mentioned in #98 to transform feature points into a new grid to allow for segmenting on a different grid from the one you are tracking on.

Note the change to the time comparisons in the segmentation code; iris times gave me a pretty good headache, but I think the solution is universal until we switch over to xarray.

• 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?
• If you have introduced a new functionality, have you added an example notebook? (Added radar/satellite segmentation tobac-tutorials#28)
• 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?

[codecov]

Codecov Report

Patch coverage: 92.30% and project coverage change: +1.29 🎉

Comparison is base (40b104a) 54.60% compared to head (44b6881) 55.90%.

❗ Current head 44b6881 differs from pull request most recent head c77396e. Consider uploading reports for the commit c77396e to get more accurate results

Additional details and impacted files

@@              Coverage Diff              @@
##           RC_v1.5.0     #242      +/-   ##
=============================================
+ Coverage      54.60%   55.90%   +1.29%     
=============================================
  Files             15       15              
  Lines           3170     3279     +109     
=============================================
+ Hits            1731     1833     +102     
- Misses          1439     1446       +7     

Flag	Coverage Δ
unittests	55.90% <92.30%> (+1.29%)	⬆️

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

Impacted Files	Coverage Δ
tobac/feature_detection.py	86.46% <ø> (+1.15%)	⬆️
tobac/tracking.py	73.20% <ø> (ø)
tobac/utils/__init__.py	100.00% <ø> (ø)
tobac/utils/internal.py	87.96% <86.04%> (-1.93%)	⬇️
tobac/utils/general.py	71.37% <96.61%> (+6.86%)	⬆️
tobac/segmentation.py	92.83% <100.00%> (+0.01%)	⬆️

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

[w-k-jones]

Looks good at first glance, my main concerns are regarding unexpected behaviour in transform_feature_points. Otherwise most of my comments are things that are probably best left for when we refactor everything for xarray in version 2.

I'll check out the branch and try it with some different data later and let you know if any issues arise

[JuliaKukulies]

Great addition @freemansw1 ! Looks overall good..just a couple of small comments and one issue where I am not sure if we have to address this in this PR (see comment about standard_name used in iris conversion)

[freemansw1]

@JuliaKukulies @w-k-jones this is ready for re-review. @lettlini how can I/we add an additional few dependencies for the notebooks? The tutorial I include here requires PyArt and a few others.

[lettlini]

@freemansw1 I think it would be a good idea to add a separate step for installing packages that are not tobac dependencies. We should move pytables there as well.

To do this you would have to add a step to the action files, e.g.

- name: Install non-tobac dependencies
       run: |
           conda install -c conda-forge --yes pytables arm_pyart

Alternatively, you could just add the packages required by the notebook to the existing Install tobac dependencies step.

Let me know if you need any further help!

[w-k-jones]

@freemansw1 @lettlini I don't think including pyart as a dependency is a good idea. When I try installing a clean environment with the tobac requirements + pyart it falls back to python 3.9, so it appears there are already some conflicts.

Would it be possible to create a file of the gridded radar output from pyart, upload this to zenodo and have the example notebook download and use this in the same manner as the original examples? We could also include the pyart code used to create the gridded dataset in a comment/markdown text to demonstrate how it was done

[freemansw1]

@freemansw1 @lettlini I don't think including pyart as a dependency is a good idea. When I try installing a clean environment with the tobac requirements + pyart it falls back to python 3.9, so it appears there are already some conflicts.

Would it be possible to create a file of the gridded radar output from pyart, upload this to zenodo and have the example notebook download and use this in the same manner as the original examples? We could also include the pyart code used to create the gridded dataset in a comment/markdown text to demonstrate how it was done

I am happy to switch to that, but I think the downside there is that the user can't just download and run the code (which I see as a pretty big downside). @lettlini is there a way to make it notebook-specific, as a compromise?

To be clear- just talking about adding pyart as a dependency for notebooks, not for the project.

[w-k-jones]

Looking good. I have still have a few concerns regarding the user understanding what's going on beneath the hood. This can be left to a later refactor if it's too much work to change quickly.

Question: What happens in segmentation if two features are assigned to the same point in the new dataset?

[lettlini]

@freemansw1 @lettlini I don't think including pyart as a dependency is a good idea. When I try installing a clean environment with the tobac requirements + pyart it falls back to python 3.9, so it appears there are already some conflicts.
Would it be possible to create a file of the gridded radar output from pyart, upload this to zenodo and have the example notebook download and use this in the same manner as the original examples? We could also include the pyart code used to create the gridded dataset in a comment/markdown text to demonstrate how it was done

I am happy to switch to that, but I think the downside there is that the user can't just download and run the code (which I see as a pretty big downside). @lettlini is there a way to make it notebook-specific, as a compromise?

To be clear- just talking about adding pyart as a dependency for notebooks, not for the project.

@w-k-jones may I ask in what order you installed Python and the other dependencies? Could you try the following:

1. create a new virtual environment
2. install the latest python version
3. install all other packages in a separate step

Edit
@freemansw1 We could define separate environments for each notebook but that would add a lot of complexity to an already complex workflow.

[w-k-jones]

@w-k-jones may I ask in what order you installed Python and the other dependencies? Could you try the following:

1. create a new virtual environment

2. install the latest python version

3. install all other packages in a separate step

Tested on two systems (OSX and linux), in both cases creating a new environment including requirements.txt and arm_pyart forces python=3.9, creating with requirements.txt and python=3.11 and then trying to install arm_pyart fails to solve the environment. Solving with mamba works for both cases and installs with python=3.11.3

[w-k-jones]

Even more complexity, on it's own conda create -n test_pyart -c conda-forge --yes --dry-run arm_pyart solves with python=3.10.9. It's a clash with iris that causes it to solve to 3.9...

[freemansw1]

OK, I think I've resolved all of the review comments again. Ready for re-review @JuliaKukulies @w-k-jones .

[JuliaKukulies]

The changes look good to me @freemansw1, thanks for your hard work on this!

The only thing that is not entirely clear for me yet is our dependency solution. So we use example_requirements.txt for the notebooks now? Somehow this seems to cause a fail in the notebook check workflow, but related to s3fs?

[freemansw1]

The only thing that is not entirely clear for me yet is our dependency solution. So we use example_requirements.txt for the notebooks now? Somehow this seems to cause a fail in the notebook check workflow, but related to s3fs?

Yes, it appears that it may be an issue with the pyart (?) dependencies not installing s3fs and fsspec versions that are compatible with each other. I've resolved this, but an actual bug was found, so I will need to resolve that next.

[w-k-jones]

Everything looks good, thanks for making the changes! Happy for this to be merged