-
Notifications
You must be signed in to change notification settings - Fork 14
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
Fix deprecation #430
Fix deprecation #430
Conversation
I fixed the error disallowing different-type outcome containers. However, that introduced the deprecation warning again. This fixes the resulting deprecations warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Increment patch, merge, and tag, please.
AH NO! Test fail!!!!!!!! |
Test fails are unrelated. Seems like a random tolerance thing related to the sum of probabilities. We can safely merge this without affecting anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test have been restarted 5 times by now but they still fail. I think the failure should be fixed. It isn't even a test failure but an @assert
statement failing. It isn't failing on main
branch.
if probs don't sum exactly to 1, then we normalize in the last step anyways.
the assert statement did actually catch a mistake that was caused elsewhere, so it should stay in and keep doing its job.
The error occurred because the sums of the counts computed using the The actual counts were missing an observation. The reason is that the cosine similarity is supposed to be bounded on Note: another alternative would be to use the Why this suddenly occurs I have not been able to track down. My best guess is that it has something to do with random number generation during testing. Anyways: it is fixed now, so I'm merging and tagging. relevant codefunction probs_and_outs_from_histogram(est::AddConstant, outcomemodel::OutcomeSpace,
cts::Counts{T, 1}, outs, x) where T
c = est.c
m = length(cts)
n = encoded_space_cardinality(outcomemodel, x) # Normalize based on *encoded* data.
probs = zeros(m)
for (k, nₖ) in enumerate(cts)
probs[k] = (nₖ + c) / (n + (c * m))
end
@assert sum(probs) ≈ 1
return Probabilities(probs, outs,)
end |
thanks! |
In #427, I fixed the error disallowing different-type outcome containers for
Probabilities
. However, that introduced the deprecation warning again. This fixes the resulting deprecations warning.