-
Notifications
You must be signed in to change notification settings - Fork 4
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
Implemented normalisation for weights with sum larger than one. Also … #185
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
403fd17
Implemented normalisation for weights with sum larger than one. Also …
BStoelzner 32b4132
add unit test
BStoelzner 0601538
update test with new syntax
BStoelzner 090c8fa
resolve conflict
BStoelzner 0e2a811
Merge branch 'main' into issue/146/mixmod-weight-normalisation-bug
BStoelzner File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
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.
This is great! There is an even simpler test that I think is more essential. Recalling that the behavior that first alerted us to the bug was that weights
[1,1,1]
led to the.pdf()
and.cdf()
yielding results identical to that for weights of[1,0,0]
, the most basic unit test for this fix would show that it indeed fixes that: when weights that sum to >1 are provided, we want to check that the .cdf() or .pdf() of the ensemble is not equal to what it would be for just the first element of the mixture model.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.
Ack, sorry, I meant this comment as a suggestion that could be made now or in the future but failed to change the status of the review to approved to actually indicate that, my bad!