-
Notifications
You must be signed in to change notification settings - Fork 17
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
Avoid NaN in transformed dists density and cdf #98
Avoid NaN in transformed dists density and cdf #98
Conversation
- evaluate the density at the boundaries. If 0, interval is open, otherwise closed
- suppress warnings from the jacobian calculation, which produce NaNs
Merge branch 'master' into avoid-nan-transformed-dists # Conflicts: # tests/testthat/test-transformations.R
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.
Thanks, looks great. Would be nice to use field()
where appropriate.
The assumption about the support being the same will work for now, but will need appropriately vectorising later for improved performance via vectorisation (#25)
Could you please also add yourself as ctb for the package if you would like?
thanks for the tip about Will make the suggested changes now |
Yes, you are right. Following the structure of lim but with logical would
be great, thanks!
…On Tue, Apr 2, 2024, 1:16 AM Ven Popov ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In R/support.R
<#98 (comment)>
:
> #'
-new_support_region <- function(x, limits = NULL) {
- vctrs::new_rcrd(list(x = x, lim = limits), class = "support_region")
+new_support_region <- function(x, limits = NULL, interval = list(c('closed','closed'))) {
we can do a logical vector, but it still needs to be logical(2L) because
the ends of the interval can be different (open-open, open-closed,
closed-open, closed-closed).
—
Reply to this email directly, view it on GitHub
<#98 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AD3BJFZKF5G3UBSXR3K7CQ3Y3FT4JAVCNFSM6AAAAABFQOV3O2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZRGM2TAMJSGI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
I've implemented your requested changes, could you please take a look? |
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.
Thanks! A few small suggestions.
Co-authored-by: Mitchell O'Hara-Wild <mitchelloharawild@users.noreply.github.com>
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.
Thanks, update the docs and it looks good to merge.
All done! |
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.
Great, thanks!
This PR improves the stability of density calculations for transformed distributions (#97). Previously density values outside of the support region for transformed distributions were NaN, because of the numeric derivative procedure. The approach here is:
interval
, which is a vector of two values , "open" or "closed", for the lower and upper bound respectivelyI added extensive tests to ensure this works properly.