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.
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
Dirichlet multinomial (continued) #4373
Dirichlet multinomial (continued) #4373
Changes from 42 commits
b7492d2
2106f7c
487fc8a
24d7ec8
4fbd1d9
685a428
ad8e77e
8fa717a
4db6b1c
01d359b
4892355
bc5f3bf
ffa705c
c801ef1
c8921ee
e801568
3483ab5
23ba2e4
fe018ec
25fd41a
28b0a62
d363f96
9b6828c
d438dfc
dde5c45
49b432d
83fbda6
9748a9d
7b20680
66c83b0
672ef56
2d5d20e
922515b
aa89d0a
2343004
a08bc51
22beead
7bad831
9bbddba
086459f
f8499d3
1cd2a9f
ef00fe1
f2ac8e9
f5dcdc3
c4e017a
d46dd50
3ab518d
cdd6d27
24447a4
c5e9b67
0bd6c3d
f919456
c082f00
ea0ae59
b451967
128d5cf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Do we want this here? Is any other distribution doing the same?
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.
I feel like it is unnecessary during runtime, since we are already testing this quite a lot in the unittests
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.
Asserts are not evil, but also not necessarily the best option. (Also see https://stackoverflow.com/a/13534633)
To validate external inputs, use
This has the advantage that a coverage check can tell you if the tests cover the exception.
If you want to validate an internal assumption and make sure that your code did not mess up, the assert is the right thing to do. It can help tremendously to debug or understand code.
In https://github.com/michaelosthege/pyrff/blob/master/pyrff/rff.py#L230-L261 I did both, because it took me weeks to understand the shapes of the code I was re-implementing there...
In this case if you're already testing it a lot maybe a comment instead of an assert is more appropriate.
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.
Yes, I think this is being sufficiently tested for reasonable inputs; I believe @ricardoV94 demonstrated already that these asserts were passed for a large variety of shapes (...even when some of the actual outputs were somewhat unintuitive).
I'm more worried about users "abusing" weirdly shaped inputs, in which case I like your explicit ShapeError to catch corner cases we didn't even think of.
On the other hand, Multinomial doesn't do this now. If we wanted to add this check I think we should do it for both distributions in parallel.
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.
I agree with this