Details
-
Type:
RFC
-
Status: Implemented
-
Resolution: Done
-
Component/s: DM
-
Labels:None
Description
meas_deblender contains the file python/lsst/meas/deblender/deblend.py, which defines SourceDeblendTask and SourceDeblendConfig.
meas_deblender also contains the file python/lsst/meas/deblender/baseline.py, which contains — amongst other things — the function deblend.
Finally, it contains the file python/lsst/meas/deblender/__init__.py, which does:
from .baseline import *
|
...
|
from .deblend import *
|
The intention here seems to be to lift the contents of baseline.py into the lsst.meas.deblender namespace. Unfortunately, the deblend() function is shadowed by deblend.py, and so isn't accessible:
[ins] In [1]: import lsst.meas.deblender
|
|
|
[ins] In [2]: lsst.meas.deblender.deblend
|
Out[2]: <module 'lsst.meas.deblender.deblend' from '/ssd/swinbank/meas_deblender/python/lsst/meas/deblender/deblend.py'>
|
|
|
[ins] In [3]: lsst.meas.deblender.baseline.deblend
|
Out[3]: <function lsst.meas.deblender.baseline.deblend ....
|
This, in itself, is a fairly minor inconvenience (anybody trying to call lsst.meas.deblender.deblend() has presumably long since given up). However, it means that documentation for deblend() is not accessible to Sphinx using the standard .. automodapi:: lsst.meas.deblender syntax.
There are a couple of ways to make these docs appear. First, one could add a .. automodapi:: lsst.meas.deblender.baseline. That works, but has the downside that the non-shadowed content of baseline.py now appears in the documentation twice. One could fix that by not lifting baseline into lsst.meas.deblender... but that's an incompatible API change.
My proposed alternative is to rename deblend.py to sourceDeblendTask.py. This is in-keeping with the spirit (if not quite the letter) of the coding standards.
Currently, SourceDeblendTask is accessible as both lsst.meas.deblender.SourceDeblendTask and lsst.meas.deblender.deblend.SourceDeblendTask. This change would remove the latter, thereby introducing a backwards-incompatible change to the API. I contend that all reasonable consumers are using the former (a contention which is supported by a brief grep of our existing code), so I expect breakage to be minimal. I therefore suggest that making this change without the normal deprecation period would be appropriate. However, I'd welcome feedback from the community in general and the DM-CCB in particular as to whether they agree.
Attachments
Issue Links
Activity
... rename it to _sourceDeblendTask.py ...
I don't feel strongly about this, but I note that that convention is not used elsewhere in this package so it seems a bit inconsistent.
+1
I support this change: the correlation between Task class names and the files they live in is not very strong, and making it stronger is good, regardless of the particular docs/API fix listed here (which is also good).
OK with me, and without deprecation period as anyone doing it the wrong (deep) way deserves what they get 
+1
I've never really been happy with this modules naming structure. The only thing I would add is that when I created meas_extensions_scarlet I unfortunately made a similar oversight, creating a deblend function inside of deblend.py. So my recommendation would be to also rename deblend.py in meas_extensions_scarlet to scarletDeblendTask.py.
Fred Moolekamp
added a comment - +1
I've never really been happy with this modules naming structure. The only thing I would add is that when I created meas_extensions_scarlet I unfortunately made a similar oversight, creating a deblend function inside of deblend.py . So my recommendation would be to also rename deblend.py in meas_extensions_scarlet to scarletDeblendTask.py . Following today's DM-CCB meeting, I understand the “board recommended” status to mean that the DM-CCB are happy for this change to be made with no deprecation period. On that basis, I'll file a triggering ticket to change both meas_deblender and meas_extensions_scarlet (thanks Fred Moolekamp) and adopt the RFC on its due date, barring any objections.
Looking at meas_extensions_scarlet more closely, I notice that it is not currently possible to use lsst.meas.extensions.scarlet.ScarletDeblendTask; rather, the only name that exists is lsst.meas.extensions.scarlet.deblend.ScarletDeblendTask. This is documented and used outside the project, so I don't think we can just change it.
I propose to adopt this RFC, with the implementation being to do the rename in meas_deblender, and to add a deprecation notice to meas_extensions_scarlet so that it can be fixed at some future date.
You could go a bit further and rename it to _sourceDeblendTask.py to make it even clearer that people aren't supposed to address is directly.