-
Notifications
You must be signed in to change notification settings - Fork 422
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
Add Unicode alias \oplus
for convolve
#1445
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1445 +/- ##
=======================================
Coverage 84.42% 84.42%
=======================================
Files 124 124
Lines 7456 7456
=======================================
Hits 6295 6295
Misses 1161 1161
Continue to review full report at Codecov.
|
FWIW in MeasureTheory we're trying to stick to categorical interpretation of I have no real stake in Distributions. But if you want things to be compatible, I hope you'll consider this. |
I think there are probably different considerations in MeasureTheory/Base since distribution objects are viewed differently there. Distributions.jl does not work with measures but random variables when transforming distributions, as the package itself does not work with measures (see also #1438). Hence I think the first question is: does the notation Side remarks unrelated to this PR: This PR does not deal with
As I said before, due to the interpretation of distributions as their corresponding random variables in these expressions there is no implicit lifting. Scalars are just scalars, no implicit Dirac measures. |
Do we use oplus in MeasureTheory? I had oplus as convolution in https://github.com/mschauer/GaussianDistributions.jl/blob/c358ae9cd074e5d7a29a68271f0032dceabc432e/src/GaussianDistributions.jl#L106 |
We've discussed (here) the possibility of using Following this "categorical imperative" 😉 is my personal preference. But as a broader guideline, I'd suggest that whenever I say this because I recall discussion of |
src/convolution.jl
Outdated
⊕(d1::Distribution, d2::Distribution) = convolve(d1, d2) | ||
@doc (@doc convolve) :⊕ | ||
# define Unicode alias | ||
const ⊕ = convolve |
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.
That's better, yes.
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.
Not sure what I did yesterday, I guess I was too tired 😂😂
It's astonishing that every other PR in Distributions ends up with a general discussion of at least partially unrelated things 😄 Discussions are very important but I think ideally we should keep discussions focused and in the right place since otherwise I fear even small improvements will take a long time. I don't think this PR is the right place to discuss things like e.g. I still think some of the more general misunderstanding in the comments above is caused by the different design and interpretation of such operations in Distributions and MeasureTheory: MeasureTheory consistently deals with distributions as measures whereas Distributions does not use this measure-theoretic interpretation. Therefore e.g. in operations with |
Test errors are unrelated, known, and caused by the RNG changes in Julia 1.7. Probably can be fixed (IMO ideally in a separate PR) by choosing a different seed or relaxing some tolerances. |
I've tried in good faith to contribute meaningfully to discussions in several repos, and you've repeatedly rebuked me that "this isn't relevant" or "this isn't the place", or "this is redundant". I'll try to stay out of your way, just let me know if you'd value my input at some point. |
It's nothing personal from my side, and hence I tried to formulate it as a general comment and to resolve some possible misunderstandings. It's just a general observation that in my experience sometimes slows down development unnecessarily and makes it more difficult to read up on old discussions (if they are scattered in different PRs and issues). I have to blame myself sometimes, e.g., if I bring up additional suggestions or observations that are not needed - and hence not completely relevant - for the PR at hand 🙂 |
I would intuitively expect I think I personally would be fine with |
convolve
for MvNormal
, add Unicode alias, and add documentation\oplus
for convolve
I found this PR a bit randomly but here is another idea:
|
I like this suggestion 👍 However, it seems orthogonal to this PR, doesn't it? Such an informative error could be added regardless of whether a Unicode alias for |
That's true! Just thought this PR could be turned around since there is still no agreement. |
That's what I did in https://github.com/JuliaStats/Distributions.jl/pull/1455😅 |
To keep this issue from tearing our community apart, I propose a compromise: circled ast :p (Could actually work, though? It's an asterisk, but the circle visually distinguishes it. Or alternatively the boxed asterisk |
This PR
simplifies the implementation ofconvolve(::MvNormal, ::MvNormal)
,⊕
,adds documentation forconvolve
and⊕
(Preview: https://juliastats.org/Distributions.jl/previews/PR1445/convolution/).It seems
convolve
is difficult to discover currently, probably due to the missing documentation. Moreover, I (and apparently some users: #1444) think the syntax could be a bit "nicer". The⊕
was suggested and discussed as an alias forconvolve
before (e.g. #142 (comment) and #919 (comment)), and I think it is a reasonable choice. Concerns against+
were e.g. that it might look like adding two distributions (#307 (comment)) and does not indicate whether the random variables are independent or not (#307 (comment)).