Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
WIP: Add keyword constructors #823
base: master
Are you sure you want to change the base?
WIP: Add keyword constructors #823
Changes from 35 commits
f1beb6b
ff8c4f1
0676a2b
b578ce6
f7fc221
62ff08c
99f6aae
1be2d75
18bad93
d929d8e
b642530
0c3d062
4208984
82de3fa
e1945b1
ce1cbe9
d1eedf4
ea9f5ea
b43e209
1559083
0f75676
611c238
689ac8e
cbdf2e0
f383f9f
05aaeb5
264a211
e8c278b
e1015c7
a3bf68b
2a39c36
1d68ae6
746aeea
9b58f24
49bb9a2
307d48b
92cf883
35a8f21
0f6e2ce
047846d
0d53426
c25c9a8
894b2eb
d13e32b
e0247d5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
for i in eachindex(x)
would be faster and more conciseThere 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.
true, but i'm trying not to change too much non-constructor code as part of this PR.
Care to make a fix as a new PR?
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.
While redeclaring everything can we drop the excessiive type constraints?
Let numbers be numbers
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.
maybe that should be its own PR.
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.
what's the rationale for dropping the type constraint (these are also useful as type signatures)
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.
It breaks the the abiility to use weirder number types.
In general
Real
is not a bad one.Worse is the Arrays, (it is just this occured first)
where something is decleared as taking an
Array
,rather than an
AbstractArray
.Because it breaks GPUs, Flux, and any of cool lazy array types.
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.
Probably better do this in another PR, this one is already big enough.
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.
Yeah, after I get this done I want to loosen up the array types.
Are there any non-
Real
number types that would be useful? The only one I can think of are Unitful ones, but even those wouldn't really work correctly (e.g. for a gamma distribution, it makes sense for the scale parameter to have a unit, but the shape one should still be unitless).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.
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.
and
https://github.com/JuliaStats/Distributions.jl/pull/823/files#diff-8c151605b43426a03834a31681aab1d1R27
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.
should be
Beta(α::T, β::T) where {T} = Beta{T}(α, β)
?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 creating an anonymous function?
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.
ordinarily, yes.