fix: first maybe materialize, then check for PlaceholderArray#3566
fix: first maybe materialize, then check for PlaceholderArray#3566pfackeldey merged 1 commit intomainfrom
Conversation
|
I never actually expected virtual arrays to materialize into placeholders in the first place, that's why I didn't do it. I thought the assertion was higher in priority (if it's a placeholder, stop now and don't run any extra lines of code). |
|
Materializing into placeholder doesn't work by construction: awkward/src/awkward/_nplikes/virtual.py Line 138 in 473029b |
|
Whether it's useful or not to materialize a PlaceholderArray is a different question. In any case, I think we should aim for logical correctness here, that's what this PR does. I'm not sure yet what we do in future with PlaceholderArrays and VirtualArrays interaction, but that doesn't stop us from making this correct already now. |
ianna
left a comment
There was a problem hiding this comment.
@pfackeldey - Good catch! Thank you for fixing it!
|
@pfackeldey - please, feel free to merge it if you are done with it. Thanks! |
This was a logical error. First, we have to materialize if it's a VirtualArray, then we should check for PlaceholderArrays. Otherwise we won't catch the case where a VirtualArray would materialize into a PlaceholderArray...