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

Avoid NaN in transformed dists density and cdf #98

Conversation

venpopov
Copy link
Contributor

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:

  • Add a new argument to support(), interval, which is a vector of two values , "open" or "closed", for the lower and upper bound respectively
  • support() for non-transformed functions automatically determines whether the interval is open or closed, by evaluating the density at the boundaries (0 -> open interval, 1 -> closed interval)
  • for transformed distributions, the support is determined by applying the transformation function to the support of the parent
  • warnings about NaN values during numeric derivatives are suppressed
  • all values below or above the support (strict or non-strict, depending on the interval type) are replcaed with 0s
  • similar for cdfs, but with 1 for values above upper bound

I added extensive tests to ensure this works properly.

Copy link
Owner

@mitchelloharawild mitchelloharawild left a 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?

R/support.R Outdated Show resolved Hide resolved
R/transformed.R Outdated Show resolved Hide resolved
R/transformed.R Outdated Show resolved Hide resolved
R/transformed.R Outdated Show resolved Hide resolved
@venpopov
Copy link
Contributor Author

venpopov commented Apr 1, 2024

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 field(). I am completely new to the vctrs data structure so did not yet know about this :)

Will make the suggested changes now

@mitchelloharawild
Copy link
Owner

mitchelloharawild commented Apr 1, 2024 via email

@venpopov
Copy link
Contributor Author

venpopov commented Apr 1, 2024

I've implemented your requested changes, could you please take a look?

Copy link
Owner

@mitchelloharawild mitchelloharawild left a 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.

R/default.R Outdated Show resolved Hide resolved
R/utils.R Outdated Show resolved Hide resolved
R/support.R Outdated Show resolved Hide resolved
R/transformed.R Outdated Show resolved Hide resolved
venpopov and others added 2 commits April 2, 2024 01:33
Co-authored-by: Mitchell O'Hara-Wild <mitchelloharawild@users.noreply.github.com>
Copy link
Owner

@mitchelloharawild mitchelloharawild left a 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.

@venpopov
Copy link
Contributor Author

venpopov commented Apr 2, 2024

All done!

Copy link
Owner

@mitchelloharawild mitchelloharawild left a comment

Choose a reason for hiding this comment

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

Great, thanks!

@mitchelloharawild mitchelloharawild merged commit f09caec into mitchelloharawild:master Apr 2, 2024
6 checks passed
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.

None yet

2 participants