Replace custom epsilons with numpy equivalent in LdaModel
#2308
Conversation
|
@horpto please expand the PR description. What problem is this solving, what's the motivation for this PR? |
| @@ -668,6 +656,7 @@ def inference(self, chunk, collect_sstats=False): | |||
| # Lee&Seung trick which speeds things up by an order of magnitude, compared | |||
| # to Blei's original LDA-C code, cool!). | |||
| integer_types = six.integer_types + (np.integer,) | |||
| epsilon = np.finfo(self.dtype).eps | |||
There was a problem hiding this comment.
I'm not sure this is a good idea. What are the guarantees for such epsilon?
If the epsilon is too close to the underflow edge, it might be silently ignored in some cases. I'd prefer an epsilon that is less ambiguous. I don't think we really care about getting the smallest possible number here.
In fact, do we need epsilon at all? It hints at some instability in the algorithm if it needs to be avoiding singularities in this way. Identifying when such singularities happen as soon as possible (is it a function of the input corpus? empty documents? something else?), and raising an exception, might be a preferable solution.
There was a problem hiding this comment.
New Epsilon already better than we have right now (bigger, we can even use 3 * eps). I agree that this is not the best solution (reason in algorithm instability), but this is a good workaround to avoid NaN values in models (at least, this will happens less often).
LGTM for me (improve overall model stability, but not prefect solution of course), wdyt @piskvorky ?
There was a problem hiding this comment.
Well, if it's an improvement we should merge it. But I'm still wary of the implications of this. Isn't it better to just raise an exception, rather than work around x / 0.0 by doing x / eps? Isn't the user screwed anyway (no exception, but nonsense results)?
Unfortunately I no longer remember why this code needs to be there :(
There was a problem hiding this comment.
Isn't it better to just raise an exception, rather than work around x / 0.0 by doing x / eps?
No, because this can be raised at any moment (for example, I train model 10h and before the end, model raises an exception, in final - time already spent and no model).
Isn't the user screwed anyway (no exception, but nonsense results)
Usually not: if no NaNs in matrices, model behaves adequately.
menshikh-iv
changed the title
LdaModel
|
Thanks @horpto 👍 |
Fix #2115