-
Notifications
You must be signed in to change notification settings - Fork 41
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
Support collecting to offset arrays #107
Conversation
) | ||
@test sa isa StructArray | ||
@test collect(sa.a) == 1:3 | ||
@test_broken sa.a isa OffsetArray |
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.
Broken due to this?
StructArrays.jl/src/collect.jl
Lines 28 to 29 in acb24c8
# temporary workaround before it gets easier to support reshape with offset axis | |
reshapestructarray(v::AbstractArray, d) = reshape(v, d) |
Codecov Report
@@ Coverage Diff @@
## master #107 +/- ##
==========================================
+ Coverage 91.22% 91.24% +0.02%
==========================================
Files 9 9
Lines 376 377 +1
==========================================
+ Hits 343 344 +1
Misses 33 33
Continue to review full report at Codecov.
|
Thanks for spotting the issue! I think at least for now I would prefer to disallow changing the shape of the iterator, my attempt to clean up the inconsistency you spotted is #110, do you think that's satisfactory? I could maybe add a check that |
Thanks for fixing this. #110 looks like a much better solution. It would be nice to add some tests as in this PR (or something similar) as well? |
In retrospect, I actually need to allow to collect with a different shape, as IndexedTables always wants to collect as a 1D vector. That is to say in the end I should add your test and make it work (which I believe it does with the current implementation in #110 ). The only doubt that I have is, in the case where the iterator does not have shape, should I pass to UPDATE: have gone for the consistent option (always passing |
This sounds like a good approach. Thanks for including the tests. |
This is something I noticed while writing #97. Not sure if you need this but I guess it's easier to fix than checking
has_offset_axes
and giving a good error message.