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 Censored distribution #1470

Merged
merged 138 commits into from
Jan 31, 2022
Merged

Add Censored distribution #1470

merged 138 commits into from
Jan 31, 2022

Conversation

sethaxen
Copy link
Contributor

This PR fixes #1468 for constructing a right-, left-, or interval-censored form of a distribution.

src/censored.jl Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2022

Codecov Report

Merging #1470 (f632f10) into master (cbbc1a5) will increase coverage by 0.54%.
The diff coverage is 99.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1470      +/-   ##
==========================================
+ Coverage   84.26%   84.80%   +0.54%     
==========================================
  Files         124      126       +2     
  Lines        7511     7778     +267     
==========================================
+ Hits         6329     6596     +267     
  Misses       1182     1182              
Impacted Files Coverage Δ
src/truncate.jl 87.62% <ø> (ø)
src/censored.jl 99.61% <99.61%> (ø)
src/common.jl 79.59% <100.00%> (ø)
src/show.jl 97.67% <100.00%> (ø)
src/truncated/discrete_uniform.jl 100.00% <100.00%> (ø)
src/univariate/continuous/cauchy.jl 84.44% <0.00%> (+2.22%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cbbc1a5...f632f10. Read the comment docs.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Thanks, I left some initial comments 🙂

src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
@mschauer
Copy link
Member

We could add the patch version increment for a release too.

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Did you build the documentation locally and checked that it looks fine?

src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
@sethaxen
Copy link
Contributor Author

Did you build the documentation locally and checked that it looks fine?

Yes, all looks good.

All changes have been incorporated and comments resolved, except for #1470 (comment).

With recent changes, I replaced some special-casing in the expectations with a utility function xexpy (similar in spirit to xlogy). This ensures everything works if the user does provide infinite bounds (we could strictly disallow this, but it could happen programmatically, and as mentioned somewhere above, it's in principle possible to have non-zero probability mass at infinity, e.g. Dirac(Inf)), hence providing an Inf bound is actually not the same thing as providing a nothing bound. This is now tested.

@sethaxen
Copy link
Contributor Author

This should be updated to use the new truncation syntax before merging.

Project.toml Outdated Show resolved Hide resolved
src/censored.jl Outdated Show resolved Hide resolved
@sethaxen
Copy link
Contributor Author

Okay, this should be up-to-date with master again.

@sethaxen
Copy link
Contributor Author

@devmotion is anything else needed before merging this?

Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for another great PR @sethaxen! I only have a minor suggestion regarding @check_args. I'll approve nevertheless, I'm fine with merging after it is addressed 🙂

src/censored.jl Outdated Show resolved Hide resolved
Co-authored-by: David Widmann <[email protected]>
@sethaxen
Copy link
Contributor Author

@devmotion okay then, should be good to merge!

@devmotion devmotion merged commit 6f74846 into JuliaStats:master Jan 31, 2022
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.

Constructing a censored distribution
4 participants