Fix Broadcast.broadcast_shape inference #313
Conversation
Codecov Report
@@ Coverage Diff @@
## master #313 +/- ##
===========================================
+ Coverage 0.00% 92.48% +92.48%
===========================================
Files 16 16
Lines 1469 1491 +22
===========================================
+ Hits 0 1379 +1379
+ Misses 1469 112 -1357
... and 15 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
This appears to be breaking. Could you look into the failing tests? |
Ah, sorry, I didn't realize there were downstream tests, I only checked the unit tests locally. How can I run the downstream tests? It looks like I need to clone the downstream and dev this branch? |
|
Yes, that's the way |
|
I guess this is only inferrable for vector broadcasting? I think that's fine |
It's not clear to me that vector broadcasting is the only thing impacted, but I'm not super familiar with this package's internals. This is only overloading cases when |
| @@ -30,13 +30,16 @@ BroadcastStyle(::PseudoBlockStyle{M}, ::BlockStyle{N}) where {M,N} = BlockStyle( | |||
|
|
|||
| # sortedunion can assume inputs are already sorted so this could be improved | |||
| sortedunion(a,b) = sort!(union(a,b)) | |||
| sortedunion_tuple(a::Tuple, b::Tuple) = (a..., b...) | |||
There was a problem hiding this comment.
Isn't this not really a union since it doesn't remove elements that a and b might have in common?
Could call it disjointsortedunion_tuple or something like that to clarify the inputs should also be disjoint.
There was a problem hiding this comment.
Agreed. Also, I'm not sure how I can fix the issues that come from this change..
There was a problem hiding this comment.
I think the only "solution" might be to somehow avoid this codepath, which I've not had enough time to try
This is an alternative to #312.
I noticed that, in the case when the union combines both entries, the result should still be correct (that's what the existing code is doing afterall). If we make tuples always combine entries then we have the benefit of
Base.Broadcast.broadcast_shapeboth being stack allocated and fully type inferred. This fixes my upstream issue (and I would prefer this solution over #312).@jishnub, does this look okay?
Closes #310.