-
Notifications
You must be signed in to change notification settings - Fork 32
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
Addressing #85 #86
Addressing #85 #86
Conversation
I think just having the dist params is fine for now, but it would probably be ideal down the line to include the transform ones as well (though potentially in a different method.) The reason I opened this issue (#85), by the way, was to check compatibility with |
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.
Is this ready to merge?
Wait! |
Should be good now, as long as the tests pass 👍 |
This PR implements
StatsBase.params
forTransformedDistribution
, andBase.maximum, Base.minimum
forUnivariateTransformedDistribution
.Up for discussion
StatsBase.params
only returns the parameters of the underlying base distribution, i.e.params(td.dist)
. In the case where in addition theBijector
has parameters, is it then expected that these are also included in the return-value ofparams
? Uncertain what makes the most sense. If so, this does increase the complexity of this PR a fair bit as it's not 100% clear how we'd do that in a nice way.