Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

assertion fail in numerics.cpp #26

Closed
fiendish opened this issue Nov 19, 2014 · 11 comments
Closed

assertion fail in numerics.cpp #26

fiendish opened this issue Nov 19, 2014 · 11 comments
Assignees
Labels

Comments

@fiendish
Copy link
Contributor

In numerics.cpp there is a pair of lines:

// FIXME: should this fail?
assert(rand_u < 1E-10);

I just hit this assertion error. I believe the answer to the question is, no, it shouldn't fail. It should just return draw. If that is the wrong return for some reason, then it is at least better than crashing out right?

@fiendish
Copy link
Contributor Author

So, I believe that the circumstance where this happened was when the model file and loaded data were not in sync with each other. (models generated on a different data set with the same columns but different number of rows). This is perhaps not a sane or valid thing to do, but then perhaps the error should happen somewhere else.

@dlovell
Copy link
Contributor

dlovell commented Dec 15, 2014

Hi fiendish,

I think crashing there is a reasonable thing to do and returning draw is not a reasonable thing to do: if the assert fails, there is something clearly wrong with the log probabilities or log partition passed in.

I think the only reasonable alternative to crashing is some "error handling", logging and "gentle" failure.

Best
Dan

@gregory-marton
Copy link
Contributor

Closing as stale / will-not-fix / working-as-intended. Please reopen if you feel like Dan's answer doesn't really address your concern. Thanks!

@riastradh-probcomp
Copy link
Contributor

We ran into this again a couple of weeks ago, on a data set that triggered several other edge cases as well. On review of the private correspondence about it, I can't find a satisfactory analysis of the source of the problem. So, although in the 20150709-riastradh-fixnumerics branch (merged in 5bf0ac3) I removed the assertion and changed it to return draw - 1 in this case -- since draw is not a valid result --, I'm reopening this until someone can investigate more closely and find what's really going on.

@gregory-marton
Copy link
Contributor

@fsaad what dataset were you using when you got this bug? How can we reproduce to debug? Can you shed light on what a valid result might be, or why draw might (though invalid) still be preferable to crashing for your use case?

@riastradh-probcomp
Copy link
Contributor

I guess I should clarify that there were two issues on two consecutive lines of code:

(a) that rand_u was too large to be readily attributed to numerical error, and
(b) that draw is not a reasonable result if rand_u was close enough to negative to be attributed to numerical error.

I fixed (b) in 5f54a37 by returning draw - 1 instead.

@fiendish, if you're still interested and could share your data set, we could take a closer look.

@fiendish
Copy link
Contributor Author

We determined that this assert error only seemed to occur for us when a
named dataset had been accidentally changed without also clearing the set
of computed models and then trying to resume model refinement with the old
models and the new data which had become misaligned. So the error message
we saw wasn't very helpful, but in these instances probable cause likely
came down to an error by our user and wasn't dependant on the data itself.

I guess I should clarify that there were two issues on two consecutive
lines of code:

(a) that rand_u was too large to be readily attributed to numerical error,
and
(b) that draw is not a reasonable result if rand_u was close enough to
negative to be attributed to numerical error.

I fixed (b) in 5f54a37
5f54a37
by returning draw - 1 instead.

@fiendish https://github.com/fiendish, if you're still interested and
could share your data set, we could take a closer look.


Reply to this email directly or view it on GitHub
#26 (comment)
.

@gregory-marton
Copy link
Contributor

draw-1 seems reasonable for small errors, overcounting the probability of the last element by only epsilon. But if these errors are so large as to not be attributable to numerical error, then isn't that a bug worth dying over? In @fiendish 's case, it turned out to be, and in @fsaad 's case it's unknown until he gets back to us. Unless we understand in some deeper way why the errors are happening, I'd be inclined to keep the assertion, so as to notice when something is badly wrong.

riastradh-probcomp added a commit that referenced this issue Jul 23, 2015
Increase confidence in the intended math of drawing categorical
samples, and diagnose Github issue #26.

Bump version to 0.1.18.
@riastradh-probcomp
Copy link
Contributor

In Crosscat 0.1.18, I reintroduced a variant of the assert, which for most cases likely puts a somewhat tighter bound on the allowable numerical error. Let's see if anyone trips on it now.

@riastradh-probcomp
Copy link
Contributor

@fiendish, if you could send us a reproducible test case that would be helpful too -- if only to make the failure in the mismatched data/model case you described more obvious.

@gregory-marton
Copy link
Contributor

No one has tripped on this lately, and a repro dataset seems unlikely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants