Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-4428

Remove Task.display()

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      As of DM-927 (included in release 9.2, end of S14), lsst.pipe.base.task.Task.display() was marked as deprecated. It should now be removed.

        Attachments

          Issue Links

            Activity

            Hide
            rowen Russell Owen added a comment -

            I think your free function is a big improvement. However, I suggest that your function only implement the dict-style access. Here are some arguments (which are, unfortunately, somewhat contradictory):

            • It promotes too many different ways to do the same thing. I'd strongly prefer that we promote only one technique in this function, as the preferred option. This will make it easier for users, since we'll end up with more consistency. (Maybe it should be a new ticket to make all tasks work the same way).
            • Existing code that uses .frame or .display doesn't need the function; the code to extract the frame is trivial.

            Despite the simplicity of .frame or .display, I think the dict technique is preferable because it easily allows different frames to show different things. Even if a task starts with a simple one-frame display, it could easily gain additional frames later.

            Regarding self._display if anetAstrometry.py; you are right. I missed the code.

            Show
            rowen Russell Owen added a comment - I think your free function is a big improvement. However, I suggest that your function only implement the dict-style access. Here are some arguments (which are, unfortunately, somewhat contradictory): It promotes too many different ways to do the same thing. I'd strongly prefer that we promote only one technique in this function, as the preferred option. This will make it easier for users, since we'll end up with more consistency. (Maybe it should be a new ticket to make all tasks work the same way). Existing code that uses .frame or .display doesn't need the function; the code to extract the frame is trivial. Despite the simplicity of .frame or .display, I think the dict technique is preferable because it easily allows different frames to show different things. Even if a task starts with a simple one-frame display, it could easily gain additional frames later. Regarding self._display if anetAstrometry.py; you are right. I missed the code.
            Hide
            swinbank John Swinbank added a comment -

            Ok, I'm happy with that approach. Thanks!

            Show
            swinbank John Swinbank added a comment - Ok, I'm happy with that approach. Thanks!
            Hide
            rowen Russell Owen added a comment -

            I agree that requiring 3 lines is a pain for every call to display. One obvious alternative is to make display silently do nothing if frame is None, but that makes the behavior less obvious to the user and reader. I wish we had a nice two-line form for "if this named display is wanted then display", but I think that's something to think about for an overhauled debug system.

            Show
            rowen Russell Owen added a comment - I agree that requiring 3 lines is a pain for every call to display. One obvious alternative is to make display silently do nothing if frame is None, but that makes the behavior less obvious to the user and reader. I wish we had a nice two-line form for "if this named display is wanted then display", but I think that's something to think about for an overhauled debug system.
            Hide
            swinbank John Swinbank added a comment -

            Function I'm planning to add is at https://github.com/lsst/base/pull/2. I'll open a (low priority) issue suggesting that other tasks should be updated to follow that standard after I'm finished here.

            Show
            swinbank John Swinbank added a comment - Function I'm planning to add is at https://github.com/lsst/base/pull/2 . I'll open a (low priority) issue suggesting that other tasks should be updated to follow that standard after I'm finished here.
            Hide
            swinbank John Swinbank added a comment -

            Merged.

            Show
            swinbank John Swinbank added a comment - Merged.

              People

              Assignee:
              swinbank John Swinbank
              Reporter:
              swinbank John Swinbank
              Reviewers:
              Russell Owen
              Watchers:
              Gregory Dubois-Felsmann, John Swinbank, Russell Owen
              Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins Builds

                  No builds found.