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
- mentioned in
-
Page Loading...
Activity
Paul Price Thanks for the heads-up. That should be fine. As long as all of the sims_GalSimInterface unit tests pass (which, I suspect, they will), this won't affect us.
The Travis tests for the GalSim PR passed, so it should be fine.
Paul Price
added a comment - The Travis tests for the GalSim PR passed, so it should be fine. I was referring to the unit tests in one of our sims packages. Regardless, I am quite certain this won't affect us.
Oh, I see; sorry. I see that sims_GalSimInterface passed in Jenkins:
sims_GalSimInterface: 12.0-2-gb27458b+2 ......................................................................................................ok (239.4 sec).
|
Paul Price
added a comment - Oh, I see; sorry. I see that sims_GalSimInterface passed in Jenkins :
sims_GalSimInterface: 12.0-2-gb27458b+2 ......................................................................................................ok (239.4 sec).
John Parejko, would you mind reviewing this? It's quite small (14 lines), though there's a catch: the patch is itself a patch, so it's a little meta.
price@price-laptop:~/LSST/external/galsim (tickets/DM-7114=) $ git sub
|
commit 24fbce360ac8c9ccc7c4719e18cd964fb12ff2fc
|
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
|
|
|
patches/DM-7114-assertion_to_exception.patch | 14 ++++++++++++++
|
1 file changed, 14 insertions(+)
|
Paul Price
added a comment - John Parejko , would you mind reviewing this? It's quite small (14 lines), though there's a catch: the patch is itself a patch, so it's a little meta.
price@price-laptop:~/LSST/external/galsim (tickets/DM-7114=) $ git sub
commit 24fbce360ac8c9ccc7c4719e18cd964fb12ff2fc
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
patches/DM-7114-assertion_to_exception.patch | 14 ++++++++++++++
1 file changed, 14 insertions(+)
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.
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?
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 */
|
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 */
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.
Reworded the message, tested through Jenkins and merged to master.
Paul Price
added a comment - Reworded the message, tested through Jenkins and merged to master. 
Scott Daniel, Simon said you're using GalSim on the sims side. Please let me know if this will trouble you.
I added a patch on tickets/
DM-7114of our galsim product, and that's working its way through Jenkins.The GalSim PR I submitted might be changed to fix the condition that's causing the assertion to fail. I don't think we care which way it goes since we've got our own fix, but we'll want to remove this fix the next time we upgrade galsim.