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

create a censored normal distribution #428

Merged
merged 13 commits into from
Sep 6, 2024

Conversation

sbidari
Copy link
Collaborator

@sbidari sbidari commented Sep 4, 2024

closes #427

@sbidari sbidari linked an issue Sep 4, 2024 that may be closed by this pull request
Copy link

codecov bot commented Sep 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.72%. Comparing base (3c5fbe7) to head (f677032).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
+ Coverage   93.51%   93.72%   +0.20%     
==========================================
  Files          41       43       +2     
  Lines        1018     1052      +34     
==========================================
+ Hits          952      986      +34     
  Misses         66       66              
Flag Coverage Δ
unittests 93.72% <100.00%> (+0.20%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sbidari sbidari self-assigned this Sep 4, 2024
@sbidari sbidari added this to the R Sprint milestone Sep 4, 2024
@SamuelBrand1
Copy link
Collaborator

SamuelBrand1 commented Sep 5, 2024

I think its worth reusing existing jax functions where possible for reducing code length and maybe getting optimization hacks.

Is it not possible to reuse https://jax.readthedocs.io/en/latest/_autosummary/jax.scipy.stats.truncnorm.logpdf.html and https://jax.readthedocs.io/en/latest/_autosummary/jax.random.truncated_normal.html#jax.random.truncated_normal ?

@SamuelBrand1
Copy link
Collaborator

SamuelBrand1 commented Sep 5, 2024

Ooops! Reread the issue, you are indeed doing censoring!

Its a bit confusing because you're doing truncated sampling within a censoring context. So maybe this should be a truncated normal distribution description despite ultimately being used to deal with the problems induced by censored observation?

@sbidari sbidari marked this pull request as ready for review September 5, 2024 14:57
Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

One quick thing to address before a full review.

pyrenew/distributions/censorednormal.py Outdated Show resolved Hide resolved
@damonbayer
Copy link
Collaborator

Does this differ in any substantial way from the example in the NumPyro docs: https://num.pyro.ai/en/latest/tutorials/censoring.html ? I have not reviewed this PR or the doc yet, but am curious if there are any major differences.

@dylanhmorris
Copy link
Collaborator

dylanhmorris commented Sep 5, 2024

Does this differ in any substantial way from the example in the NumPyro docs: https://num.pyro.ai/en/latest/tutorials/censoring.html ? I have not reviewed this PR or the doc yet, but am curious if there are any major differences.

  1. Docs do it as a modeling problem using only bundled distributions; this creates a numpyro.distributions.Distribution subclass to handle it directly. I strongly favor the second approach where possible for modularity / texting / reusability.
  2. Does not use a boolean array to indicate which values are censored and instead treats any values at or outside the given censoring bounds as censored.

Would be a nice feature for numpyro to have semi-automated creation of censored distributions from base distributions (as it currently has has for truncated distributions). But as far as I can see that does not yet exist.

@damonbayer damonbayer added the pyrenew related to pyrenew internals label Sep 6, 2024
@damonbayer damonbayer removed this from the 🦖 Rajasaurus milestone Sep 6, 2024
Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

Missing the line where we define self._support

pyrenew/distributions/censorednormal.py Outdated Show resolved Hide resolved
sbidari and others added 2 commits September 6, 2024 13:42
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

Unit test for the support property?

pyrenew/distributions/censorednormal.py Show resolved Hide resolved
Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

A couple suggested additions to the support testing, otherwise looks good. Thanks @sbidari!

test/test_censorednormal.py Show resolved Hide resolved
test/test_censorednormal.py Show resolved Hide resolved
Co-authored-by: Dylan H. Morris <dylanhmorris@users.noreply.github.com>
Copy link
Collaborator

@dylanhmorris dylanhmorris left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @sbidari!

@dylanhmorris dylanhmorris merged commit 1e204f6 into main Sep 6, 2024
8 checks passed
@dylanhmorris dylanhmorris deleted the 427-create-a-censorednormal-distribution branch September 6, 2024 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pyrenew related to pyrenew internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create a CensoredNormal distribution
4 participants