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

Remove afw APIs deprecated in DM-17566

    XMLWordPrintable

Details

    • Story
    • Status: Done
    • Resolution: Done
    • None
    • afw, meas_algorithms
    • None
    • 8
    • AP F20-5 (October), AP F20-6 (November)
    • Alert Production
    • No

    Description

      A number of afw APIs were formally deprecated in DM-17566. It was stated that they will be removed after the release of v20.0. This ticket is for that removal.

      As well as afw this will include at least meas_algorithms.

      Attachments

        Issue Links

          Activity

            krzys Krzysztof Findeisen added a comment - - edited

            This is a duplicate of DM-22276; I suggest closing my copy, since it doesn't have a link to the release issue.

            krzys Krzysztof Findeisen added a comment - - edited This is a duplicate of DM-22276 ; I suggest closing my copy, since it doesn't have a link to the release issue.
            tjenness Tim Jenness added a comment -

            My search was not good enough. Sorry about the duplication.

            tjenness Tim Jenness added a comment - My search was not good enough. Sorry about the duplication.

            I'm not sure, but from a superficial reading I don't think it's currently possible to execute this ticket as defined.

            Specifically, DM-17566 deprecated overloads of afwMath.BackgroundControl.__init__ which take a style parameter. Per this ticket, they must therefore be removed before release 21.0.0.

            However, lsst.meas.algorithms.SubtractBackgroundTask.fitBackground() takes an optional algorithm parameter. I believe that this parameter is only meaningful in the presence of the (deprecated) style parameter to BackgroundControl.__init__. However, this interface is not marked as deprecated, and so can't be removed.

            I conclude that we are stuck between a rock and a hard place — we have to drop the style parameter because it has been deprecated, but we can't because it'll break non-deprecated interfaces.

            I think, therefore, we have to make a new ticket to deprecated the current interface to [lsst.meas.algorithms.SubtractBackgroundTask.fitBackground() before the 20.0.0 release.

            Am I correct? I'd appreciate a sanity check, because it's late and I might have confused myself!

            swinbank John Swinbank added a comment - I'm not sure, but from a superficial reading I don't think it's currently possible to execute this ticket as defined. Specifically, DM-17566 deprecated overloads of afwMath.BackgroundControl.__init__ which take a style parameter . Per this ticket, they must therefore be removed before release 21.0.0. However, lsst.meas.algorithms.SubtractBackgroundTask.fitBackground() takes an optional algorithm parameter. I believe that this parameter is only meaningful in the presence of the (deprecated) style parameter to BackgroundControl.__init__ . However, this interface is not marked as deprecated, and so can't be removed. I conclude that we are stuck between a rock and a hard place — we have to drop the style parameter because it has been deprecated, but we can't because it'll break non-deprecated interfaces. I think, therefore, we have to make a new ticket to deprecated the current interface to [lsst.meas.algorithms.SubtractBackgroundTask.fitBackground() before the 20.0.0 release. Am I correct? I'd appreciate a sanity check, because it's late and I might have confused myself!
            swinbank John Swinbank added a comment - - edited

            The above is a bit rambling, so let me try and condense it down:

            Currently, one can run (in pseudocode):

            bg = SubtractractBackgroundTask().fitBackground(some_image, ...)
            bg.getImageF()
            

            After the APIs described on this ticket are removed, I believe that will no longer be possible: one will always need to give an argument to bg.getImageF() in the above.

            I therefore don't think we can remove these APIs until SubtractractBackgroundTask.fitBackground has been through a deprecation cycle (and it isn't currently marked as deprecated).

            swinbank John Swinbank added a comment - - edited The above is a bit rambling, so let me try and condense it down: Currently, one can run (in pseudocode): bg = SubtractractBackgroundTask().fitBackground(some_image, ...) bg.getImageF() After the APIs described on this ticket are removed, I believe that will no longer be possible: one will always need to give an argument to bg.getImageF() in the above. I therefore don't think we can remove these APIs until SubtractractBackgroundTask.fitBackground has been through a deprecation cycle (and it isn't currently marked as deprecated).
            erykoff Eli Rykoff added a comment -

            Can we at least fix up our own uses of this API?

            erykoff Eli Rykoff added a comment - Can we at least fix up our own uses of this API?

            We'll have to.

            Off the top of my head, I think the most straightforward way to do so is to implement a SubtractBackgroundTask.fitBackgroundNew method, switch our code to use that, then deprecate the old one. That seems annoyingly clunky: better suggestions welcome.

            swinbank John Swinbank added a comment - We'll have to. Off the top of my head, I think the most straightforward way to do so is to implement a SubtractBackgroundTask.fitBackgroundNew method, switch our code to use that, then deprecate the old one. That seems annoyingly clunky: better suggestions welcome.
            krzys Krzysztof Findeisen added a comment - - edited

            I'm not sure I see what the problem is. I agree that the algorithm keyword in fitBackground should be deprecated before the old BackgroundControl constructor is removed, but if we have removed getImageF(), then this keyword would have no practical effect even if the old BackgroundControl constructor is still around, so it might as well be a dummy.

            In other words, I think the following is a reasonable sequence of events:

            1. deprecate fitBackground(..., algorithm=....)
            2. remove getImageF() and BackgroundControl(style, ...) as previously planned. algorithm can no longer affect calls to the remaining overloads of getImageF().
            3. remove algorithm keyword, possibly in the version after
            krzys Krzysztof Findeisen added a comment - - edited I'm not sure I see what the problem is. I agree that the algorithm keyword in fitBackground should be deprecated before the old BackgroundControl constructor is removed, but if we have removed getImageF() , then this keyword would have no practical effect even if the old BackgroundControl constructor is still around, so it might as well be a dummy. In other words, I think the following is a reasonable sequence of events: deprecate fitBackground(..., algorithm=....) remove getImageF() and BackgroundControl(style, ...) as previously planned. algorithm can no longer affect calls to the remaining overloads of getImageF() . remove algorithm keyword, possibly in the version after

            Hmm. I think you are right, krzys. I was worried that (ignoring the algorithm keyword) the interface to fitBackground could be described as “returns a thing that you can call .getImageF() on”, but — while that's true — it's not relevant here: that's already covered by the warning on getImageF itself.

            Given that, I think the procedure you describe above is fine (and I don't think it needs an RFC).

            Tangentially, I do wonder a bit what happens if the (deprecated) “style” given to BackgroundControl is different from the argument to getImageF — which takes precedence? I suppose I need to go and look at the code.

            swinbank John Swinbank added a comment - Hmm. I think you are right, krzys . I was worried that (ignoring the algorithm keyword) the interface to fitBackground could be described as “returns a thing that you can call .getImageF() on”, but — while that's true — it's not relevant here: that's already covered by the warning on getImageF itself. Given that, I think the procedure you describe above is fine (and I don't think it needs an RFC). Tangentially, I do wonder a bit what happens if the (deprecated) “style” given to BackgroundControl is different from the argument to getImageF — which takes precedence? I suppose I need to go and look at the code.

            Yes, I worried about that a bit during DM-17566. As you might expect, the immediately provided argument takes precedence, but there's still room for consistency bugs. I think we'll be fine if we are careful to change all the Background-related APIs at once. Probably.

            krzys Krzysztof Findeisen added a comment - Yes, I worried about that a bit during DM-17566 . As you might expect, the immediately provided argument takes precedence, but there's still room for consistency bugs. I think we'll be fine if we are careful to change all the Background -related APIs at once. Probably.

            Filed DM-23076 to deprecate fitBackground(..., algorithm=....), and marked it as blocking release 20.0.0.

            swinbank John Swinbank added a comment - Filed DM-23076 to deprecate fitBackground(..., algorithm=....) , and marked it as blocking release 20.0.0.

            This ticket no longer covers Background-related APIs, which were deprecated on DM-17566 and restored on DM-24565.

            krzys Krzysztof Findeisen added a comment - This ticket no longer covers Background -related APIs, which were deprecated on DM-17566 and restored on DM-24565 .
            Parejkoj John Parejko added a comment - - edited Jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/32966/pipeline
            Parejkoj John Parejko added a comment -

            Note for those who come later: while sorting this out, I ran in to quite a few segfaults in sub-packages due to calls to now non-existent pybind11 methods. It's unfortunately, but maybe inevitable with removal of things in C++. Hopefully our tests caught them all, but no guarantees.

            Parejkoj John Parejko added a comment - Note for those who come later: while sorting this out, I ran in to quite a few segfaults in sub-packages due to calls to now non-existent pybind11 methods. It's unfortunately, but maybe inevitable with removal of things in C++. Hopefully our tests caught them all, but no guarantees.
            Parejkoj John Parejko added a comment -

            krzys: Are you still ok to review this? I believe we want it in the next weekly, if we can. There are 12 PRs (all should be on the Jira sidebar; ignore the very old merged one that referenced this ticket somehow). The biggest is afw, with ~470 lines removed. Most of the others are pretty small.

            Jenkins with ci_hsc was successful, so I think I caught everything that there was to catch.

            Good news, afw does not build with any deprecation warnings any more! `scons -s` is completely clean until the tests/docs run.

            Parejkoj John Parejko added a comment - krzys : Are you still ok to review this? I believe we want it in the next weekly, if we can. There are 12 PRs (all should be on the Jira sidebar; ignore the very old merged one that referenced this ticket somehow). The biggest is afw, with ~470 lines removed. Most of the others are pretty small. Jenkins with ci_hsc was successful, so I think I caught everything that there was to catch. Good news, afw does not build with any deprecation warnings any more! `scons -s` is completely clean until the tests/docs run.

            Ok, I think I've reviewed all the PRs this time.

            I'm a bit worried about the segfaults you mentioned, since I don't think those should be happening. Did these occur even on a clean build? Should we warn users?

            krzys Krzysztof Findeisen added a comment - Ok, I think I've reviewed all the PRs this time. I'm a bit worried about the segfaults you mentioned, since I don't think those should be happening. Did these occur even on a clean build? Should we warn users?
            Parejkoj John Parejko added a comment - - edited New post-review jenkins run: https://ci.lsst.codes/blue/organizations/jenkins/stack-os-matrix/detail/stack-os-matrix/33020/pipeline
            Parejkoj John Parejko added a comment - - edited

            As for the segfaults: that probably is worth a warning in the next release. How do we make that happen (ktl?)?

            I'm not sure what you mean by "on a clean build": they happened while running tests in subpackages that did successfully compile.

            I just reconstructed an example: checkout ip_diffim master (pre-this ticket) and scons it against this ticket of afw+daf_persistence+meas_algorithms+meas_base+meas_deblender_obs_base (i.e. its dependencies with this branch: maybe most easily done via `rebuild -r tickets/DM-22814 ip_diffim` and then checking out ip_diffim master) I get crashes in `tests/test_assessSpatialKernelVisitor.py` and `tests/test_assessSpatialKernelVisitor.py` (on Ubuntu 18.04, gcc 7.5.0). It probably should not have successfully built because of `src/KernelSolution.cc`; I'm not sure why that didn't fail in the compiler.

            Parejkoj John Parejko added a comment - - edited As for the segfaults: that probably is worth a warning in the next release. How do we make that happen ( ktl ?)? I'm not sure what you mean by "on a clean build": they happened while running tests in subpackages that did successfully compile. I just reconstructed an example: checkout ip_diffim master (pre-this ticket) and scons it against this ticket of afw+daf_persistence+meas_algorithms+meas_base+meas_deblender_obs_base (i.e. its dependencies with this branch: maybe most easily done via `rebuild -r tickets/ DM-22814 ip_diffim` and then checking out ip_diffim master) I get crashes in `tests/test_assessSpatialKernelVisitor.py` and `tests/test_assessSpatialKernelVisitor.py` (on Ubuntu 18.04, gcc 7.5.0). It probably should not have successfully built because of `src/KernelSolution.cc`; I'm not sure why that didn't fail in the compiler.
            ktl Kian-Tat Lim added a comment -

            The segfaults occur if you use old code against the new?  That's OK, we've never promised that that would work (hence the unitary mega-release).

            ktl Kian-Tat Lim added a comment - The segfaults occur if you use old code against the new?  That's OK, we've never promised that that would work (hence the unitary mega-release).
            ktl Kian-Tat Lim added a comment -

            It would be nice if uses of now-gone interfaces failed at the pybind11 or C++ compiler levels, but perhaps overloading/templates makes that impossible?

            In any case, it's a good suggestion that the release note saying that the interfaces have been removed make very clear that uses of them will cause problems.  Please send suggested text to gcomoretto to incorporate.

            ktl Kian-Tat Lim added a comment - It would be nice if uses of now-gone interfaces failed at the pybind11 or C++ compiler levels, but perhaps overloading/templates makes that impossible? In any case, it's a good suggestion that the release note saying that the interfaces have been removed make very clear that uses of them will cause problems.  Please send suggested text to gcomoretto to incorporate.
            krzys Krzysztof Findeisen added a comment - - edited

            It would be nice if uses of now-gone interfaces failed at the pybind11 or C++ compiler levels, but perhaps overloading/templates makes that impossible?

            It's worse than that. If I understand Parejkoj's description correctly, the thing that should not have compiled was calling convolve(OutImageT, InImageT, KernelT, bool) when the only remaining overload is convolve(OutImageT, InImageT, KernelT, ConvolutionControl). It turns out that ConvolutionControl is implicitly convertable from a bool.

            I think the only way to avoid this is to tighten the style guide to require explicit constructors in general; implicit construction through default arguments is too easy to overlook.

            krzys Krzysztof Findeisen added a comment - - edited It would be nice if uses of now-gone interfaces failed at the pybind11 or C++ compiler levels, but perhaps overloading/templates makes that impossible? It's worse than that. If I understand Parejkoj 's description correctly, the thing that should not have compiled was calling convolve(OutImageT, InImageT, KernelT, bool) when the only remaining overload is convolve(OutImageT, InImageT, KernelT, ConvolutionControl) . It turns out that ConvolutionControl is implicitly convertable from a bool . I think the only way to avoid this is to tighten the style guide to require explicit constructors in general; implicit construction through default arguments is too easy to overlook.
            Parejkoj John Parejko added a comment -

            Oh, I was wondering if there was an implicit constructor hiding in there somewhere. krzys: do you mind filing tickets about that for ConvolutionControl and for updating the style guide? I agree that we should just require explicit constructors always, but I'm not sure how to best word those things.

            krzys and I are working on wording to send to gcomoretto re: interface removal.

            Parejkoj John Parejko added a comment - Oh, I was wondering if there was an implicit constructor hiding in there somewhere. krzys : do you mind filing tickets about that for ConvolutionControl and for updating the style guide? I agree that we should just require explicit constructors always, but I'm not sure how to best word those things. krzys and I are working on wording to send to gcomoretto re: interface removal.
            ktl Kian-Tat Lim added a comment -

            While I can't think of a case where we'd have a problem with having every constructor marked explicit, it might do less violence to the code base (and achieve compliance more rapidly) to only require it for constructors with 0 or 1 non-defaulted arguments.

            ktl Kian-Tat Lim added a comment - While I can't think of a case where we'd have a problem with having every constructor marked explicit , it might do less violence to the code base (and achieve compliance more rapidly) to only require it for constructors with 0 or 1 non-defaulted arguments.
            Parejkoj John Parejko added a comment - - edited

            For the record, krzys sent an note to Gabriele regarding the release notes and filed DM-27424 about the `convolve` implicit constructor.

            Final jenkins run successful, and everything rebased and merged. Phew.

            Parejkoj John Parejko added a comment - - edited For the record, krzys sent an note to Gabriele regarding the release notes and filed DM-27424 about the `convolve` implicit constructor. Final jenkins run successful, and everything rebased and merged. Phew.

            People

              Parejkoj John Parejko
              tjenness Tim Jenness
              Krzysztof Findeisen
              Eli Rykoff, John Parejko, Kian-Tat Lim, Krzysztof Findeisen, Tim Jenness
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.