Fix subtle floating point bug in ProbabilityTree #1080
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… and raise an exception rather than abort
|
Note: This is listed to merge into releases/2.2. Once merged, I'll also cherry-pick it to master. But we should probably cut a bugfix release in the 2.2 series with this fix. |
rmjarvis
commented
May 13, 2020
| # But I'm finding S in the reference, and U in info. | ||
| c = info.dtype[name].char | ||
| c = 'S' if c == 'U' else c | ||
| assert dt.char == c, "column %s is the wrong type"%name |
There was a problem hiding this comment.
This change is unrelated to the topic of this PR. There was apparently a change in behavior of either meds or fitsio that caused this test to start failing. I already fixed it on master, but not yet on releases/2.2.
rmjarvis
commented
May 13, 2020
| @@ -170,19 +175,22 @@ namespace galsim { | |||
| // stops quickly with the large flux Elements on the left. | |||
| double half_tot = absFlux/2.; | |||
| double leftSum=0.; | |||
| for (; leftSum < half_tot; ++mid) leftSum += std::abs((*mid)->getFlux()); | |||
| for (; leftSum < half_tot && mid < end; ++mid) { | |||
There was a problem hiding this comment.
This specifically is where the bug was. Added && mid < end here, since the leftSum with rounding can remain less than half_tot, even though 2*half_tot is supposed to be the sum of everything (in exact arithmetic).
jchiang87
approved these changes
May 13, 2020
rmjarvis
added
bug report
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@jchiang87 found a bug in drawing InterpolatedImage objects when there were a bunch of pixels with values around 1.e-16 -- 1.e-12 of the total flux where it would seg fault on the draw command.
It turned out to be due to a subtle bug in the ProbabilityTree code where some sums could have rounding errors that caused some of the totals to get out of sync, eventually leading a pointer to go one higher than it should have been able to. Once that happened, it went into a loop that sent it off into the depths of the computer's memory dereferencing things willy nilly until it seg faulted.
Anyway, the solution was to add an extra check to make sure that the variable (
mid) doesn't go past the part of the array being worked on (end), rather than relying on the floating point math to ensure that, since it doesn't necessarily do so, apparently.Along the way, I was also getting frustrated trying to get the asserts to trigger, since the python compiler automatically sets
-DNDEBUG, which turns off asserts, and it's hard to undo that it turns out. Plus, the regular assert bombs out with an abort, which isn't very python-friendly.So I rolled my own
assertstatement (copied from TreeCorr actually), which will now always run some checks regardless of NDEBUG status to make sure we don't get seg faults if things aren't what we expect. And if they do hit, it raises a runtime_error, rather than abort, so a bit less catastrophic when running from python.We have some other
asserts throughout the code, and I checked that they are all not doing any significant calculations. Most of them check for non-NULL pointers and similar things that would lead to badness if they were false. So probably worth keeping them all turned on all the time.There is also an
xassertwe use in various places which is (still) not run unless DEBUGLOGGING is turned on. So those are mostly for trying to track down bugs. Some of these do have arithmetic in them, but they turn into a no op in normal running, so that's ok.