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
adapt RPS #277
adapt RPS #277
Changes from 32 commits
3fff094
70e1e9b
072f50f
00d24b2
9eea7c3
ddd51ca
e0b177a
eca1d7e
09b1e86
51cb1b0
35396ed
724853d
0e62ec5
b2352b2
36e4eeb
d3e5656
f43c9c9
9d5443b
e8a5d14
ed12fad
da79880
a012fe8
243f1a6
500ee62
371898e
47fd871
04e8bf5
2256ac2
022bbdf
b875ddf
3dd943d
86176cc
20610bb
1b3e36b
15923cc
6991802
904a0f3
0267df1
efffb79
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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'm wondering now if these might be confusing. As in the docstring, the cdf bins are actually all bounded on the left by
-np.inf
, but these are correct if one is thinking about the pdf. I'm not sure what's clearest for the user:'[-np.inf, 0.33), [0.33, 0.66), [0.66, np.inf]'
; or'[-np.inf, 0.33), [-np.inf, 0.66), [-np.inf, np.inf]'
Happy for you to decide what you think is the clearest, but we should probably be consistent between the docstring and
_assign_rps_category_bounds
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.
Dos the question is whether we think in cdf or pdf? Cumulative bins or single size bins? Although rps is computed based on cdfs, I would still call it category_edges and therefore use the first choice of category edges as coords.
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.
Sounds good, but then we should maybe also change the docstring to be consistent. That is, back to "For example, specifying category_edges = [0,1] will compute the rps for bins [-inf, 0), [0, 1) and [1, inf)"
Then I happy to merge, thanks!