Uploaded image for project: 'Request For Comments'
  1. Request For Comments
  2. RFC-892

Remove fpSets from return struct of SourceDetectionTask

    XMLWordPrintable

    Details

    • Type: RFC
    • Status: Adopted
    • Resolution: Unresolved
    • Component/s: DM

      Description

      SourceDetectionTask.run() in meas_algorithms includes an fpSets item in the return struct, which is just a copy of the result from detectFootprints(). This was added during a refactor by Paul Price in 2018, and has a # Backward compatibility comment on it. I've seen confusion about the contents of the SourceDetectionTask return structure (partly caused by it not being fully documented, which I'm fixing on a user-branch ticket); the fpSets item is completely redundant, and people should just use the positive/negative/etc. items of the run return struct instead.

      We don't have a good way to deprecate return values from functions and Tasks, so I am proposing to just remove this item from the return struct, and fix our internal usage of it. I don't know of a better approach for dealing with external users and return struct contents, but we have some other places where we should remove things from a return struct (e.g. DM-35940), and no good way to manage that deprecation.

        Attachments

          Issue Links

            Activity

            Hide
            ktl Kian-Tat Lim added a comment -

            In this case, a drastic but common mechanism would be to create a new method, perhaps with a versioned name, that has a different effective return type (not in the mypy sense). Unfortunately, run() is built into our PipelineTask interface definition, so that mechanism is not available.

            The only thing I can think of is to return either a class instance specific to this method or a more-generic DeprecatingStruct that behaves like the usual lsst.pipe.base.Struct but that warns on access to the fpSets component (or other specified component(s)).

            Show
            ktl Kian-Tat Lim added a comment - In this case, a drastic but common mechanism would be to create a new method, perhaps with a versioned name, that has a different effective return type (not in the mypy sense). Unfortunately, run() is built into our PipelineTask interface definition, so that mechanism is not available. The only thing I can think of is to return either a class instance specific to this method or a more-generic DeprecatingStruct that behaves like the usual lsst.pipe.base.Struct but that warns on access to the fpSets component (or other specified component(s)).
            Hide
            jbosch Jim Bosch added a comment - - edited

            CCB agrees that in this case we can get by without a deprecation, but in the future it would be good to have functionality (preferably in Struct itself) to allow for deprecation, as it's unlikely future removals would be as innocuous as this one.

            Personally, I'm also attracted to the idea of using method-specific dataclasses instead of Struct, not just for deprecations, but as a general preference - but I don't feel strongly enough to try to change a well-established pattern right now, even for new code (if someone else does, and wants to make another RFC, you'll probably get my vote). 

            Show
            jbosch Jim Bosch added a comment - - edited CCB agrees that in this case we can get by without a deprecation, but in the future it would be good to have functionality (preferably in Struct itself) to allow for deprecation, as it's unlikely future removals would be as innocuous as this one. Personally , I'm also attracted to the idea of using method-specific dataclasses instead of Struct, not just for deprecations, but as a general preference - but I don't feel strongly enough to try to change a well-established pattern right now, even for new code (if someone else does, and wants to make another RFC, you'll probably get my vote). 
            Hide
            Parejkoj John Parejko added a comment -

            Adopting with implementation ticket DM-37468.

            Show
            Parejkoj John Parejko added a comment - Adopting with implementation ticket DM-37468 .

              People

              Assignee:
              Parejkoj John Parejko
              Reporter:
              Parejkoj John Parejko
              Watchers:
              Ian Sullivan, Jim Bosch, John Parejko, Kian-Tat Lim, Merlin Fisher-Levine, Paul Price, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

                Dates

                Created:
                Updated:
                Planned End:

                  Jenkins

                  No builds found.