Closed venpopov closed 3 months ago
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
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 https://github.com/mitchelloharawild/distributional/pull/98#discussion_r1546376712 :
'
-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 https://github.com/mitchelloharawild/distributional/pull/98#discussion_r1546376712, 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?
All done!
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.