Fix type instability when setindex!!
#549
Conversation
src/utils.jl
Outdated
| function BangBang.possible( | ||
| ::typeof(BangBang._setindex!), ::C, ::T, ::Vararg | ||
| ) where {C<:AbstractArray,T<:AbstractArray} | ||
| return BangBang.implements(setindex!, C) && | ||
| promote_type(eltype(C), eltype(T)) <: eltype(C) | ||
| end |
There was a problem hiding this comment.
This is type piracy, isn't it? It shouldn't be defined in DynamicPPL but in BangBang itself.
There was a problem hiding this comment.
Yes, @torfjelde already open a PR at JuliaFolds/BangBang.jl#238.
There was a problem hiding this comment.
This repo is inactive. JuliaFolds was forked to JuliaFolds2, so the PR has to be opened in https://github.com/JuliaFolds2/BangBang.jl.
There was a problem hiding this comment.
We should be able to get a timely review and release there.
There was a problem hiding this comment.
Ah, that make sense.
Can confirm https://github.com/JuliaRegistries/General/blob/d04b3fa7e9c6283040b6cea5bb53d3e78954d6fe/B/BangBang/Package.toml#L3
@torfjelde do you want migrate the PR to JuliaFolds2?
There was a problem hiding this comment.
Also verified locally that the BangBang PR should fix the issue
There was a problem hiding this comment.
Oh wow, I was not aware of this! Okay, that's quite good to know 👍
And yeah, we should do that 👍
One thing though: we probably don't need the other overloads with this Vararg version, right?
There was a problem hiding this comment.
I think removing the other should be safe 👍.
Maybe if the JuliaFold2 folks are responsive, this PR can be just removing all the BangBang.possible
Pull Request Test Coverage Report for Build 6623738317
💛 - Coveralls |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #549 +/- ##
==========================================
- Coverage 82.76% 80.29% -2.48%
==========================================
Files 25 25
Lines 3180 3156 -24
==========================================
- Hits 2632 2534 -98
- Misses 548 622 +74
☔ View full report in Codecov by Sentry. |
|
Just making a note: comparing the size and dimensions of the target and value arrays is bad, because the existence of vector of arrays |
Is this referring to my PR to BangBang.jl? If so, isn't this dealt with first through the check of the eltypes? EDIT: Realized what you meant. I think we should just restrict to |
Haha, it was actually for myself, sorry for the confusion, I was trying to make the |
|
@devmotion do you know who to ping for the PR over at JuliaFolds2? |
|
Mason would be my first guess. |
|
@torfjelde, @devmotion do we want to merge this for now and remove the type-piracy code after the JuliaFolds2 PR merged? |
|
Don't merge this. We should just take my PR from JuliaFolds.
…On Tue, Oct 31, 2023, 12:08 PM Hong Ge ***@***.***> wrote:
***@***.**** approved this pull request.
—
Reply to this email directly, view it on GitHub
<#549 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACUPZZCEDV2DATTPJJOISB3YCDS2LAVCNFSM6AAAAAA6NHSW42VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMBWGEZTSOJYHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
@sunxd3 do you want to have a go at moving the changes from my PR into one for DynamicPPL? |
|
Done, although I didn't copy the tests form JuliaFolds2 |
|
Dope 👍 We should also include the tests though |
|
@torfjelde another look? |
Temporary fix for #530