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

Guard against assertion failure

    Details

    • Type: Bug
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: galsim
    • Labels:
      None
    • Story Points:
      0.5
    • Epic Link:
    • Sprint:
      DRP F16-3
    • Team:
      Data Release Production

      Description

      measureCoaddSources can hit an assertion failure in GalSim:

      python: src/hsm/PSFCorr.cpp:731: void galsim::hsm::find_ellipmom_1(galsim::ConstImageView<double>, double, double, double, double, double, double&, double&, double&, double&, double&, double&, double&, boost::shared_ptr<galsim::hsm::HSMParams>): Assertion `iy1 <= iy2' failed.
      

      This is due to GalSim using asserts rather than throwing an exception. We fixed this on the HSC side (7a73869), but it didn't come over because we're using GalSim as a dependency rather than sucking it into the package.

      I thought I reported this upstream at one point...

        Attachments

          Issue Links

            Activity

            Hide
            Parejkoj John Parejko added a comment -

            Seems fine, though I'd like the exception to say something about what bounds (iy1, iy2 or what those mean higher up) and/or include the actual values. I see the debug statements above the throw, but it's nice for the exception message to be self contained.

            Show
            Parejkoj John Parejko added a comment - Seems fine, though I'd like the exception to say something about what bounds (iy1, iy2 or what those mean higher up) and/or include the actual values. I see the debug statements above the throw, but it's nice for the exception message to be self contained.
            Hide
            Parejkoj John Parejko added a comment -

            Oh, wait. The assert is still there at the bottom: shouldn't the assert(iy1 <= iy2); line be removed, since it's already been handled by the except?

            Show
            Parejkoj John Parejko added a comment - Oh, wait. The assert is still there at the bottom: shouldn't the assert(iy1 <= iy2); line be removed, since it's already been handled by the except?
            Hide
            price Paul Price added a comment -

            Thanks John Parejko.

            I would add more information to the exception, except that that's not done for any of the other exceptions in that file, and so I'm not sure what technique they would prefer for string formatting and I don't want to add any #include files just for this. If you insist I'll do something, but I think I would prefer to leave it.

            I've removed the assert:

            pprice@tiger-sumire:~/LSST/external/galsim (tickets/DM-7114 %=) $ git sub-patch
            commit 26dc8bf89928ae3b68dd7fb8c23271941511a4c6
            Author: Paul Price <price@astro.princeton.edu>
            Date:   Wed Aug 3 12:36:24 2016 -0400
             
                replace assertion with exception
                
                This is an exceptional condition that can occur sometimes
                (because it certainly does occur!), rather than something
                that should bring the world to a screaming halt.
                
                Reported upstream as PR #784:
                  https://github.com/GalSim-developers/GalSim/pull/784
             
            diff --git a/patches/DM-7114-assertion_to_exception.patch b/patches/DM-7114-assertion_to_exception.patch
            new file mode 100644
            index 0000000..b64f1ff
            --- /dev/null
            +++ b/patches/DM-7114-assertion_to_exception.patch
            @@ -0,0 +1,15 @@
            +diff --git a/src/hsm/PSFCorr.cpp b/src/hsm/PSFCorr.cpp
            +index a9b3260..21b92b1 100644
            +--- a/src/hsm/PSFCorr.cpp
            ++++ b/src/hsm/PSFCorr.cpp
            +@@ -728,7 +728,9 @@ namespace hsm {
            +         if (iy2 > ymax) iy2 = ymax;
            +         dbg<<"y1,y2 = "<<y1<<','<<y2<<std::endl;
            +         dbg<<"iy1,iy2 = "<<iy1<<','<<iy2<<std::endl;
            +-        assert(iy1 <= iy2);
            ++        if (iy1 > iy2) {
            ++             throw HSMError("Bounds don't make sense");
            ++        }
            + 
            +         //
            +         /* Use these pointers to speed up referencing arrays */
            

            Show
            price Paul Price added a comment - Thanks John Parejko . I would add more information to the exception, except that that's not done for any of the other exceptions in that file, and so I'm not sure what technique they would prefer for string formatting and I don't want to add any #include files just for this. If you insist I'll do something, but I think I would prefer to leave it. I've removed the assert : pprice@tiger-sumire:~/LSST/external/galsim (tickets/DM-7114 %=) $ git sub-patch commit 26dc8bf89928ae3b68dd7fb8c23271941511a4c6 Author: Paul Price <price@astro.princeton.edu> Date: Wed Aug 3 12:36:24 2016 -0400   replace assertion with exception This is an exceptional condition that can occur sometimes (because it certainly does occur!), rather than something that should bring the world to a screaming halt. Reported upstream as PR #784: https://github.com/GalSim-developers/GalSim/pull/784   diff --git a/patches/DM-7114-assertion_to_exception.patch b/patches/DM-7114-assertion_to_exception.patch new file mode 100644 index 0000000..b64f1ff --- /dev/null +++ b/patches/DM-7114-assertion_to_exception.patch @@ -0,0 +1,15 @@ +diff --git a/src/hsm/PSFCorr.cpp b/src/hsm/PSFCorr.cpp +index a9b3260..21b92b1 100644 +--- a/src/hsm/PSFCorr.cpp ++++ b/src/hsm/PSFCorr.cpp +@@ -728,7 +728,9 @@ namespace hsm { + if (iy2 > ymax) iy2 = ymax; + dbg<<"y1,y2 = "<<y1<<','<<y2<<std::endl; + dbg<<"iy1,iy2 = "<<iy1<<','<<iy2<<std::endl; +- assert(iy1 <= iy2); ++ if (iy1 > iy2) { ++ throw HSMError("Bounds don't make sense"); ++ } + + // + /* Use these pointers to speed up referencing arrays */
            Hide
            Parejkoj John Parejko added a comment -

            Ok, if string formatting is a pain, can you at least have the message say what the bad bounds are? I don't know what iy1,iy2 mean here, but something like: "Vertical data bounds (iy1,iy2) don't make sense." would be better, I think.

            Show
            Parejkoj John Parejko added a comment - Ok, if string formatting is a pain, can you at least have the message say what the bad bounds are? I don't know what iy1,iy2 mean here, but something like: "Vertical data bounds (iy1,iy2) don't make sense." would be better, I think.
            Hide
            price Paul Price added a comment -

            Reworded the message, tested through Jenkins and merged to master.

            Show
            price Paul Price added a comment - Reworded the message, tested through Jenkins and merged to master.

              People

              • Assignee:
                price Paul Price
                Reporter:
                price Paul Price
                Reviewers:
                John Parejko
                Watchers:
                John Parejko, Paul Price, Scott Daniel
              • Votes:
                0 Vote for this issue
                Watchers:
                3 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel