Skip to content

fix: simplify unmasked array#3518

Merged
ianna merged 2 commits intomainfrom
ianna/fix_umnasked_simplfy
May 30, 2025
Merged

fix: simplify unmasked array#3518
ianna merged 2 commits intomainfrom
ianna/fix_umnasked_simplfy

Conversation

@ianna
Copy link
Copy Markdown
Member

@ianna ianna commented May 22, 2025

Fixes Issue 1 in #3403

An Unmasked array has no missing values by definition. It is still an option type, though.
That’s why, when its content was an Unmasked array, the simplified method was introducing an IndexedOptionArray node.

@martindurant - FYI

@martindurant
Copy link
Copy Markdown
Contributor

Thanks for coming back to this!

The case should be testable, right?

Copy link
Copy Markdown
Member Author

@ianna ianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agoose77 - I am not sure if we are using the is_option type for unmasked arrays. That is why I decided to add an extra flag. Another question is if we should drop all nested Unmasked arrays recursively.

@agoose77
Copy link
Copy Markdown
Collaborator

@ianna UnmaskedArray behaves as an option, and so we will be taking slow paths if we're only testing is_option. My feeling with Awkward is that we're dealing with a closed set of node types (or at least, closed set of families), so adding a new flag that's helpful to identify a member of that set is fine!

I don't entirely follow your question about dropping recursively; we can repeatedly simplify UnmaskedArray(UnmaskedArray(...)), but we can't simplify through type boundaries like lists.

@ianna ianna merged commit d84c7ea into main May 30, 2025
54 of 56 checks passed
@ianna ianna deleted the ianna/fix_umnasked_simplfy branch May 30, 2025 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants