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

Add Multinomial Distribution and Modify Examples #127

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add Multinomial Distribution and Modify Examples #127

wants to merge 3 commits into from

Conversation

matteolepur
Copy link
Collaborator

This will fail because of a runtime error caused by the IntSliceSampler receiving invalid parameters (0,0) in the DiscreteUnif generator. Can anyone help me with this?

Copy link
Contributor

@alexandrebouchard alexandrebouchard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few problems identified. There might be others.

for (int i: 0..< numberofSuccesses.size()) {
if (probabilities.get(i) < 0.0 || probabilities.get(i) > 1.0) return NEGATIVE_INFINITY
if (numberofSuccesses.get(i) < 0.0) return NEGATIVE_INFINITY
if (numberofTrials <= 0 || numberofSuccesses.get(i) < numberofTrials) return NEGATIVE_INFINITY
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A first problem: numberofSuccesses.get(i) < numberofTrials should be numberofSuccesses.get(i) > numberofTrials

if (probabilities.get(i) < 0.0 || probabilities.get(i) > 1.0) return NEGATIVE_INFINITY
if (numberofSuccesses.get(i) < 0.0) return NEGATIVE_INFINITY
if (numberofTrials <= 0 || numberofSuccesses.get(i) < numberofTrials) return NEGATIVE_INFINITY
sum0 += (numberofSuccesses.get(i) * log(probabilities.get(i)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 26 could cause 0 * -infty = NaN, which is not the right behaviour

}
}

logf(numberofSuccesses, probabilities, numberofTrials) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This second block should be combined with the first since it uses the same arguments.

@@ -77,6 +78,18 @@ class Examples {

public val List<Instance<? extends Model>> all = new ArrayList

<<<<<<< HEAD
public val multinomial = add(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test cases with probabilities equal to zero and one

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

Successfully merging this pull request may close these issues.

2 participants