Split up utils module similar to `v2.0-dev`

[freemansw1]

This should address https://github.com/tobac-project/tobac/issues/122 and bring our utils module closer to what we have in v2.0-dev. This should pair nicely with https://github.com/tobac-project/tobac/pull/179 and is another (small) step toward bridging the gap between v2.0-dev and v1.x.

Note that the way that I've done this should preserve all existing code. We will probably want to depreciate public use of some of these functions (e.g., why would a user want to call add_coordinates), but that is not this PR.

(this is a re-do of #190)

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

[freemansw1]

I should note that the only actual code change that should be here is the deletion of the two commented out functions in uitls.py. I didn't see much of a point to keeping those around.

[codecov[bot]]

Codecov Report

Base: 40.82% // Head: 41.04% // Increases project coverage by +0.22% :tada:

Coverage data is based on head (71bdcda) compared to base (10288bc). Patch coverage: 61.68% of modified lines in pull request are covered.

Additional details and impacted files

@@              Coverage Diff              @@
##           RC_v1.5.0     #191      +/-   ##
=============================================
+ Coverage      40.82%   41.04%   +0.22%     
=============================================
  Files             11       14       +3     
  Lines           2332     2317      -15     
=============================================
- Hits             952      951       -1     
+ Misses          1380     1366      -14     

Flag	Coverage Δ
unittests	41.04% <61.68%> (+0.22%)	:arrow_up:

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

Impacted Files	Coverage Δ
tobac/utils/mask.py	13.75% <13.75%> (ø)
tobac/utils/general.py	64.38% <64.38%> (ø)
tobac/utils/internal.py	97.75% <97.75%> (ø)
tobac/feature_detection.py	80.76% <100.00%> (ø)
tobac/utils/__init__.py	100.00% <100.00%> (ø)

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

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

[snilsn]

I haven't gone through this in detail yet, but one thing I wondered:

There is now a separate __init__.py for tobac.utils, but there are still imports from tobac.utils in the original __init__.py.

That means I could access the same function in three different ways, e. g.

import tobac
tobac.mask_features()
tobac.utils.mask_features()
tobac.utils.mask.mask_features()

would all call the same function. Is that the way it's meant to be? I can imagine that this could be confusing.

[JuliaKukulies]

@snilsn I think having a separate __init__.py file is not a bad idea for submodules like the new utils that contain multiple files and functions and is also the module that is the most likely to rapidly grow. I see your point that it is a bit confusing to have three different ways of calling a function and converting utils into a package itself would allow us to just import utils in tobac.__init__.py which then facilitates calling the functions, independent in which submodule of utils they are. I could, however, imagine that @freemansw1 also kept the original imports to not break anybody's code when they usually call the functions with tobac.mask_features() etc. Is that right?

@freemansw1 Agreed with the removal of the functions that were commented out.

[freemansw1]

Yes, what @JuliaKukulies says is exactly the reason that I structured the code the way that I did. Any functions that were exposed via import tobac are still exposed to the user through this mechanism, and this should only break code if people tried to import tobac.utils and call a function not exposed through import tobac directly from there. The functions that would apply to are largely functions that I doubt users are calling anyway (e.g., get_label_props_in_dict).

I'm not opposed to deprecating that way of calling functions, and encouraging users to switch to importing tobac.utils instead, but I think that's a different PR (and I don't immediately know how to do that).

That said, this doesn't necessarily avoid breaking everyone's code. If a user is directly using a function from tobac.utils that is not exposed now, their code would break without a deprecation warning. As far as I can tell, the functions that this would apply to are:

• tobac.utils.get_label_props_in_dict
• tobac.utils.get_indices_of_labels_from_reg_prop_dict

Maybe it makes sense to import those functions from utils/__init__ to really avoid breaking anyone's code?

[snilsn]

Thank for this carification @JuliaKukulies and @freemansw1, that makes a lot of sense. I agree that deprecating this way of calling functions is probably not necessary and certainly not urgent.

[freemansw1]

After thinking about it some, I've added the two functions mentioned above to tobac.utils such that no code (should) be broken.

I've also addressed @snilsn's comment on deleting the not implemented function.

[JuliaKukulies]

I have realized that changing this structure affects the auto-generated API reference and does not show any of the new submodules in tobac.utils, see https://tobac--191.org.readthedocs.build/en/191/tobac.html#module-tobac.segmentation

Do we also just simply need to add the submodule to tobac.rst @freemansw1 ?

[freemansw1]

Good catch, @JuliaKukulies . I've added the tobac.utils.general and tobac.utils.mask modules to the API reference, but not the tobac.utils.internal, on the theory that internal utils shouldn't be used by users anyway. That said, we don't explicitly label these with _ to mark them as internal, so I'm happy to add them to the documentation if we think that's what is best.

[freemansw1]

OK, with #179 merged, I'm re-requesting reviews and will merge after those go through.