Implement lastindex for ragged arrays#501
Conversation
src/vector_of_array.jl
Outdated
| return lastindex(VA.u) | ||
| elseif d < ndims(VA) | ||
| isempty(VA.u) && return 0 | ||
| return _is_ragged_dim(VA, d) ? RaggedEnd{d}() : size(VA.u[1], d) |
There was a problem hiding this comment.
Does this not give inference issues?
There was a problem hiding this comment.
Hm yes, might be. As I said, this was AI-generated. I'll see if I can fix that.
There was a problem hiding this comment.
instead of using types if it's runtime, so enum then I think the strategy is fine.
There was a problem hiding this comment.
I'm not sure I understand. RaggedEnd should be an enum instead of a struct? If yes, what would be the values?
There was a problem hiding this comment.
or just 0 and interpret what zero means.
There was a problem hiding this comment.
To be honest, I only understand half of what is going on here, but aren't we loosing information (the d) when we return 0 instead of RaggedEnd{d}()? I tried making it work with the help of Claude, but there were always tests failing.
There was a problem hiding this comment.
yeah I see. The point though is to just make it be some kind of runtime value instead of a compile time value. So instead of a type with d, just like a tuple (true, d) for ragged true/false.
There was a problem hiding this comment.
Ok, thanks for the clarification! So, should Base.lastindex always return a tuple then? Also in the non-ragged case?
There was a problem hiding this comment.
I made a suggestion in 36561a0. This uses runtime values, but still returns RaggedEnd. As far as I understand we cannot return tuples because we need to overload + and - for that, which we obviously do not want to do for a general tuple.
src/vector_of_array.jl
Outdated
| return RaggedEnd(Int(d)) | ||
| else | ||
| return 1 |
There was a problem hiding this comment.
type unstable, add JET testing to this
There was a problem hiding this comment.
Yeah, that's also why I asked
So, should Base.lastindex always return a tuple then? Also in the non-ragged case?
above. In 7d9e008, I now let lastindex always return a RaggedEnd to make lastindex type stable (because again, I think we need to return an object of a custom struct for the ragged case). For non-ragged arrays, this is now encoded in RaggedEnd as dim = 0. This required some special handling for this case to behave the same as before. AI helped me again for that. I also added some more tests.
|
Main thing here is still JET tests |
Sounds good. I can add that. I did run JET tests locally and they failed due to other reasons. I propose to add and fix the issues in another PR before to not mix different things in this PR. Is that fine with you? |
|
yes that's fine |
2 participants
Checklist
contributor guidelines, in particular the SciML Style Guide and
COLPRAC.
Additional context
This is an AI-generated solution to allowing the use of
endfor raggedVectorOfArrays//heterogeneous inner array sizes. As also noted in #454 (comment), previously this could either give wrong results or give aBoundsError.Closes #267.