Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork70
Implementlastindex for ragged arrays#501
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:master
Are you sure you want to change the base?
Conversation
src/vector_of_array.jl Outdated
| returnlastindex(VA.u) | ||
| elseif d<ndims(VA) | ||
| isempty(VA.u)&&return0 | ||
| return_is_ragged_dim(VA, d)?RaggedEnd{d}():size(VA.u[1], d) |
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.
Does this not give inference issues?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
instead of using types if it's runtime, so enum then I think the strategy is fine.
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.
I'm not sure I understand.RaggedEnd should be anenum instead of a struct? If yes, what would be the values?
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.
or just 0 and interpret what zero means.
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.
To be honest, I only understand half of what is going on here, but aren't we loosing information (thed) when we return 0 instead ofRaggedEnd{d}()? I tried making it work with the help of Claude, but there were always tests failing.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, thanks for the clarification! So, shouldBase.lastindex always return a tuple then? Also in the non-ragged case?
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.
I made a suggestion in36561a0. This uses runtime values, but still returnsRaggedEnd. 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.
| returnRaggedEnd(Int(d)) | ||
| else | ||
| return1 |
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.
type unstable, add JET testing to this
Uh oh!
There was an error while loading.Please reload this page.
Checklist
contributor guidelines, in particular theSciML 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.