Skip to content

fix: keep EmptyArray in remove_structure#2219

Merged
jpivarski merged 2 commits intomainfrom
agoose77/fix-completely-flatten-emptyarray
Feb 7, 2023
Merged

fix: keep EmptyArray in remove_structure#2219
jpivarski merged 2 commits intomainfrom
agoose77/fix-completely-flatten-emptyarray

Conversation

@agoose77
Copy link
Copy Markdown
Collaborator

@agoose77 agoose77 commented Feb 7, 2023 •
edited
Loading

Fixes #2207 by preserving EmptyArray in ak._do.remove_structure. This means that code that previously did not handle EmptyArray would now be expected to. I don't think there are many places where this is actually a problem; EmptyArray already handles reduction, and flattening almost immediately returns the result.

@agoose77 agoose77 requested a review from jpivarski February 7, 2023 11:54
@agoose77 agoose77 temporarily deployed to docs-preview February 7, 2023 12:11 — with GitHub Actions Inactive
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 7, 2023

Codecov Report

Merging #2219 (133781b) into main (339896f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/operations/ak_flatten.py 92.50% <ø> (ø)
...rc/awkward/operations/ak_merge_union_of_records.py 88.70% <ø> (ø)
src/awkward/contents/emptyarray.py 75.00% <100.00%> (+1.04%) ⬆️

jpivarski
jpivarski approved these changes Feb 7, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@jpivarski jpivarski left a comment

Choose a reason for hiding this comment

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

This means that code that previously did not handle EmptyArray would now be expected to.

The thing I would do in this situation is to grep for all the calls to remove_structure and check that all of those call sites would be able to handle an EmptyArray. The fact that the test suite passes is half as good as that.

@jpivarski
Copy link
Copy Markdown
Member

jpivarski commented Feb 7, 2023

It seems to be entirely in ak.flatten and ak.ravel:

% fgrep -r '._remove_structure(' src/awkward
src/awkward/contents/bitmaskedarray.py:            return self.project()._remove_structure(backend, options)
src/awkward/contents/listarray.py:        return self.to_ListOffsetArray64(False)._remove_structure(backend, options)
src/awkward/contents/unmaskedarray.py:            return self.project()._remove_structure(backend, options)
src/awkward/contents/indexedarray.py:        return self.project()._remove_structure(backend, options)
src/awkward/contents/unionarray.py:                ._remove_structure(backend, options)
src/awkward/contents/listoffsetarray.py:            contents = content._remove_structure(backend, options)
src/awkward/contents/indexedoptionarray.py:            return self.project()._remove_structure(backend, options)
src/awkward/contents/bytemaskedarray.py:            return self.project()._remove_structure(backend, options)
src/awkward/contents/regulararray.py:            contents = content._remove_structure(backend, options)
src/awkward/contents/recordarray.py:                out.extend(content[: self._length]._remove_structure(backend, options))
src/awkward/_do.py:        arrays = layout._remove_structure(

% fgrep -r 'do.remove_structure(' src/awkward
src/awkward/operations/ak_flatten.py:        out = ak._do.remove_structure(layout, function_name="ak.flatten")
src/awkward/operations/ak_ravel.py:    out = ak._do.remove_structure(layout, function_name="ak.ravel", drop_nones=False)

@jpivarski
Copy link
Copy Markdown
Member

jpivarski commented Feb 7, 2023

Both of them send the result to mergemany and nothing else. mergemany can handle EmptyArray, so there's no issue there.

@jpivarski jpivarski merged commit aaf36a5 into main Feb 7, 2023
@jpivarski jpivarski deleted the agoose77/fix-completely-flatten-emptyarray branch February 7, 2023 17:28
@agoose77
Copy link
Copy Markdown
Collaborator Author

agoose77 commented Feb 7, 2023

I think my wording could have been better here; I am generally hesitant to change anything that we likely are making assumptions about. I did the same checks you did to be as confident as possible. I am glad you found the same results! (occasionally I have missed something like this despite thinking otherwise!)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jpivarski jpivarski jpivarski approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Different behavior for awkward 2.x on ak.flatten with unknown type

2 participants

@agoose77 @jpivarski