Possibly obsolete functions/wrappers in segmentation.py

[snilsn]

Hello everybody,

@JuliaKukulies and me have independently stumbled upon the functions segmentation_3D(), segmentation_2D(), watershedding_3D() and watershedding_2D() in segmentation.py.

These don't seem to have any real use, since they only call the segmentation() function itself, and, in case of the watershedding ones, set the method to watershedding, which is the default anyway. Writing docstrings for them feels a bit redundant. Maybe these can be removed in a future release?

In v2 they are already gone, so this would also be a step towards compatibility.

On the other hand, I just noticed that the functions are used in all the example notebooks of v1, so at least those would need to be adjusted.

What are your thoughts on that?

[JuliaKukulies]

Yes, I stumbled upon that function during the review of #127, because a new input parameter was added to the functions segmentation_2D and segmentation_3D: seed_3D_flag. Currently the default for seed_3D_flag is "column" for both cases 3D and 2D, but I was wondering whether the functions segmentation_2D and segmentation_3D were thought to be different? @galexsky and @freemansw1, do we really need this differentiation for the 3D implementation?

[freemansw1]

I think these functions are leftover from when they were separate functions; in our PR we only modified them to maintain compatibility. I am happy for us to deprecate them, but they are used in multiple people's code, so we should deprecate and then remove in ~v2.0.

[mheikenfeld]

The functions look the way they do (as really simple wrappers around the segmentation function) because they were once introduced for backwards compatibility at an early stage of development. Deprecating them in the future and adding a deprecation warning now sound like a good plan now!

[freemansw1]

Resolved with #147 (I'm not sure why GitHub didn't auto-close this?)

[freemansw1]

Reopening this issue as I realized that import tobac does not import segmentation(), only the depreciated functions. We'll need to do another PR for this, but that should be simple.

[freemansw1]

@JuliaKukulies this seems to be a bigger issue than I initially thought. If I add segmentation() to __init__.py, that breaks importing tobac.segmentation as a module, instead importing the function, meaning it is difficult to get to the rest of the tobac.segmentation module. Any ideas on the best way to resolve this? We could point people to start calling tobac.segmentation.segmentation, but that doesn't seem ideal either.

We could also move segmentation() to be called watershedding or something? Any thoughts?

[JuliaKukulies]

True, that is an important point and it is certainly no alternative to just overwrite the name of the module, because I think it is important that the tobac.segmentation module is also accessible itself.

From the two alternatives you are suggesting (and I cannot really think of another solution now), I personally would prefer to rename segmentation(), because I am always getting confused with python functions or classes with the same name as their modules (see datetime.datetime and co )

[freemansw1]

Yes, agreed with you. Given that it would be a substantial change, maybe it's a discussion for the developers' meeting this week? At the very least, I'm hesitant to do this before a major version change (to 1.4.0). Maybe the path forward on this is to revert the depreciation on segmentation_2D and segmentation_3D before releasing v1.3.2?

[JuliaKukulies]

Yes, lets bring this up and I think you are right to wait and revert the depreciation warnings so that they are released in the same version.

[freemansw1]

Okay, I've put in a PR to revert these changes (#154) and we will put this on the agenda for the developers meeting this week.

My suggestion as stated above is to create a new function watershedding inside of segmentation.py, move the segmentation code there, and depreciate all other wrapper functions (segmentation_2D, segmentation_3D, watershedding_2D, watershedding_3D). I don't see a need to remove the depreciated functions until v1.6 or later though as they have a low maintenance cost while generating headaches for users. Tagging @w-k-jones @fsenf so that they see this discussion.