refactor: replace `simplify_optiontype` and `simplify_uniontype` with `simplified` classmethod #1928

jpivarski · 2022-11-30T20:04:15Z

Instead of constructing non-canonical arrays and then simplifying them into something canonical, these @classmethods only construct canonical arrays from the start.

The next PR after this will enforce canonicity (level 1) in the Content constructors. This is the first half of #1910.

📚 The documentation for this PR will be available at https://awkward-array.readthedocs.io/en/jpivarski-replace-simplify-method-with-classmethod-constructor/ once Read the Docs has finished building 🔨

…method-constructor

codecov · 2022-11-30T20:13:16Z

Codecov Report

Merging #1928 (c226220) into main (1157d22) will decrease coverage by 0.14%.
The diff coverage is 76.03%.

Additional details and impacted files

Impacted Files	Coverage Δ
src/awkward/_broadcasting.py	`93.39% <ø> (ø)`
src/awkward/_slicing.py	`85.84% <0.00%> (-0.27%)`	⬇️
src/awkward/operations/ak_mask.py	`95.23% <ø> (ø)`
src/awkward/operations/ak_nan_to_none.py	`23.52% <0.00%> (ø)`
src/awkward/operations/ak_to_categorical.py	`92.15% <ø> (-0.30%)`	⬇️
src/awkward/index.py	`84.10% <15.38%> (-6.48%)`	⬇️
src/awkward/forms/indexedform.py	`80.85% <41.17%> (-8.76%)`	⬇️
src/awkward/forms/bitmaskedform.py	`82.22% <46.66%> (-3.86%)`	⬇️
src/awkward/forms/bytemaskedform.py	`82.14% <46.66%> (-4.16%)`	⬇️
src/awkward/forms/unmaskedform.py	`81.94% <46.66%> (-4.95%)`	⬇️
... and 32 more

src/awkward/forms/bitmaskedform.py

src/awkward/forms/form.py

…ey're right after the normal constructor, now.

…ptiontype').

jpivarski · 2022-11-30T21:58:47Z

src/awkward/_slicing.py

-        next = item.simplify_optiontype()
-        return normalise_item_nested(next)
+        return normalise_item_nested(item)


This is one of the few that doesn't replace a simplify_optiontype with a simplified classmethod—not a pure refactor—so it's possible that non-canonical layouts pass through here.

However, I think this was sanitizing user input, and after constructors forbid non-canonical structures, user input can't be insane. So while this opens the door to error, the next PR will shut it.

jpivarski · 2022-11-30T21:59:45Z

src/awkward/contents/bitmaskedarray.py

-            return [self.simplify_optiontype()]
+            return [self]


Here's another unbalanced simplify_optiontype → simplified pair, but in the future, self can't be non-canonical.

jpivarski · 2022-11-30T22:00:28Z

src/awkward/contents/bytemaskedarray.py

-            return [self.simplify_optiontype()]
+            return [self]


Another unbalanced simplify_optiontype → simplified pair.

jpivarski · 2022-11-30T22:01:35Z

src/awkward/contents/indexedoptionarray.py

-            return [self.simplify_optiontype()]
+            return [self]


Another unbalanced simplify_optiontype → simplified pair.

jpivarski · 2022-11-30T22:02:13Z

src/awkward/contents/unmaskedarray.py

-            return [self.simplify_optiontype()]
+            return [self]


Another unbalanced simplify_optiontype → simplified pair.

jpivarski · 2022-11-30T22:02:27Z

src/awkward/operations/ak_nan_to_none.py

-            return continuation().simplify_optiontype()
+            return continuation()


Another unbalanced simplify_optiontype → simplified pair.

jpivarski · 2022-11-30T22:02:44Z

src/awkward/operations/ak_to_categorical.py

-            if layout.is_option:
-                layout = layout.simplify_optiontype()


The last unbalanced simplify_optiontype → simplified pair.

jpivarski · 2022-11-30T22:03:29Z

@agoose77 The PyLint test is failing because nplike was removed from the argument list. (I'm not sure how...)

agoose77 · 2022-11-30T22:09:15Z

@jpivarski ak_from_buffers takes a backend object now instead of nplike, which means the argument name has changed. I thought we'd okayed that w.r.t people like dask-awkward who will need to update. If you're still happy with #1922, I can make PRs for dask-awkward et al

This commit enables backends to pass through `regularize_backend`, and ensures that we pass a backend (not nplike) to `from_buffers`

agoose77 · 2022-11-30T22:21:38Z

I took your comment as a green light to push some fixes. Feel free to revert / rebase them if you're not happy.

src/awkward/_backends.py

…iontype').

jpivarski · 2022-11-30T23:47:36Z

src/awkward/_slicing.py

-        attempt = item.simplify_uniontype()
-        if isinstance(attempt, ak.contents.UnionArray):
-            raise ak._errors.wrap_error(
-                TypeError(
-                    "irreducible unions (different types at the same level in an array) can't be used as slices"
-                )
+        raise ak._errors.wrap_error(
+            TypeError(
+                "irreducible unions (different types at the same level in an array) can't be used as slices"
            )
-
-        return normalise_item_nested(attempt)
+        )


This one is not a simplify_uniontype → simplified pair because it's sanitizing input, just in case (like the option-type exceptions noted above).

jpivarski · 2022-11-30T23:48:23Z

src/awkward/contents/unionarray.py

-        for i, self_cont in enumerate(contents):
+        for i, self_cont in enumerate(self_contents):


This had been an error in the translation of simplify_uniontype to simplified. I don't see any similar errors.

jpivarski · 2022-11-30T23:49:41Z

src/awkward/contents/unionarray.py

    def _getitem_next_jagged_generic(self, slicestarts, slicestops, slicecontent, tail):
-        simplified = self.simplify_uniontype()
-        if isinstance(simplified, ak.contents.UnionArray):
+        if isinstance(self, ak.contents.UnionArray):
            raise ak._errors.index_error(
                self,
                ak.contents.ListArray(
                    slicestarts, slicestops, slicecontent, parameters=None
                ),
                "cannot apply jagged slices to irreducible union arrays",
            )
-        return simplified._getitem_next_jagged(
-            slicestarts, slicestops, slicecontent, tail
-        )
+        return self._getitem_next_jagged(slicestarts, slicestops, slicecontent, tail)


This one is not a simplify_uniontype → simplified pair because it's sanitizing input. There was a test providing insane inputs, so the simplified part of the pair went into the testing code.

jpivarski · 2022-11-30T23:50:55Z

src/awkward/contents/unionarray.py

-        simplified = self.simplify_uniontype(True, True)
+        simplified = type(self).simplified(
+            self._tags,
+            self._index,
+            self._contents,
+            parameters=self._parameters,
+            backend=self._backend,
+            merge=True,
+            mergebool=True,
+        )


This one re-simplifies the inputs because the merging strategy is more aggressive than normal (mergebool=True), so this is to prevent any changes in behavior.

jpivarski · 2022-11-30T23:51:09Z

src/awkward/contents/unionarray.py

-        simplified = self.simplify_uniontype(True, True)
+        simplified = type(self).simplified(
+            self._tags,
+            self._index,
+            self._contents,
+            parameters=self._parameters,
+            backend=self._backend,
+            merge=True,
+            mergebool=True,
+        )


jpivarski · 2022-11-30T23:51:18Z

src/awkward/contents/unionarray.py

-        simplified = self.simplify_uniontype(mergebool=True)
+        simplified = type(self).simplified(
+            self._tags,
+            self._index,
+            self._contents,
+            parameters=self._parameters,
+            backend=self._backend,
+            mergebool=True,
+        )


jpivarski · 2022-11-30T23:51:29Z

src/awkward/contents/unionarray.py

-        simplified = self.simplify_uniontype(mergebool=True)
+        simplified = type(self).simplified(
+            self._tags,
+            self._index,
+            self._contents,
+            parameters=self._parameters,
+            backend=self._backend,
+            mergebool=True,
+        )


jpivarski · 2022-11-30T23:51:39Z

src/awkward/contents/unionarray.py

-        simplified = self.simplify_uniontype(mergebool=True)
+        simplified = type(self).simplified(
+            self._tags,
+            self._index,
+            self._contents,
+            parameters=self._parameters,
+            backend=self._backend,
+            mergebool=True,
+        )


jpivarski · 2022-11-30T23:52:04Z

src/awkward/operations/ak_concatenate.py

-            out = out.simplify_uniontype(merge=merge, mergebool=mergebool)
+            out = type(out).simplified(
+                out._tags,
+                out._index,
+                out._contents,
+                parameters=out._parameters,
+                backend=out._backend,
+                merge=merge,
+                mergebool=mergebool,
+            )


See above (mergebool might be True).

…method-constructor

jpivarski · 2022-12-01T02:48:21Z

@agoose77, this is now fully implemented and ready for review. Each step is called out in code comments, and the last step is this: the simplified classmethods now convert attempts to make

option[union[X, Y, ...]]

into

union[option[X], option[Y], ...]

That is, Arrow convention is now forced. The way that option-type push-down is implemented (there are many choices!) is this: one of the UnionArray contents, tag_for_missing (preferentially a RecordArray) is selected to be wrapped with IndexedOptionArray.simplified. If it's another option-type, more merging will happen. All of the other contents are wrapped with UnmaskedArray.simplified, which is a trivial operation, regardless of whether the thing being wrapped is option-type.

We can change the way tag_for_missing is selected at any time. Maybe it should be preferentially something that's already an option-type?

What this PR does not implement is the canonicity requirements in the Content constructors. That's why #1910 is not closed by this PR. But now that we have a constructor that makes canonical versions of any Content, the next PR just has to enforce those requirements, discover if we are using non-canonical constructions anywhere, and build them with simplified instead.

src/awkward/contents/unionarray.py

src/awkward/operations/ak_where.py

Co-authored-by: Angus Hollands <[email protected]>

jpivarski added 5 commits November 30, 2022 12:45

Added @classmethod simplified (and copy) to all Forms.

dac274e

Is adding a 'simplified' option to to_IndexedArray64 right?

cfcd73c

No, I don't think so.

c521dad

Merge branch 'main' into jpivarski/replace-simplify-method-with-class…

57bad8e

…method-constructor

Implemented, but have not tested, 'simplified' classmethods.

86c8561

jpivarski added the pr-next-release Required for the next release label Nov 30, 2022

jpivarski changed the title ~~Replace simplify_optiontype and simplify_uniontype with simplified classmethod~~ refactor: replace simplify_optiontype and simplify_uniontype with simplified classmethod Nov 30, 2022

agoose77 reviewed Nov 30, 2022

View reviewed changes

src/awkward/forms/bitmaskedform.py Outdated Show resolved Hide resolved

src/awkward/forms/form.py Outdated Show resolved Hide resolved

jpivarski added 2 commits November 30, 2022 14:32

Every Content and Form should have a 'simplified' constructor, and th…

ea7e51e

…ey're right after the normal constructor, now.

Removed all calls of 'simplify_optiontype' (except within 'simplify_o…

f8ad58d

…ptiontype').

jpivarski commented Nov 30, 2022

View reviewed changes

fix: use backend instead of nplike

36e979f

This commit enables backends to pass through `regularize_backend`, and ensures that we pass a backend (not nplike) to `from_buffers`

agoose77 reviewed Nov 30, 2022

View reviewed changes

src/awkward/_backends.py Show resolved Hide resolved

Removed all calls of 'simplify_uniontype' (except within 'simplify_un…

7bea551

…iontype').

jpivarski commented Nov 30, 2022

View reviewed changes

jpivarski and others added 5 commits November 30, 2022 17:53

Merge branch 'main' into jpivarski/replace-simplify-method-with-class…

cea7a30

…method-constructor

Redundant statements of the defaults.

077b4b6

Removed 'simplify_optiontype' and 'simplify_uniontype'.

3c3078c

Use 'cls' where possible (one of which fixed an untested bug).

c6a2f92

The 'simplified' classmethod also prevents option-of-union now.

501d344

jpivarski marked this pull request as ready for review December 1, 2022 02:37

agoose77 requested changes Dec 1, 2022

View reviewed changes

jpivarski and others added 6 commits December 1, 2022 11:41

Fixed unnecessary double-construction.

fb68bfd

nplike → index_nplike

6ec98c3

Co-authored-by: Angus Hollands <[email protected]>

nplike → index_nplike

6d9079a

Co-authored-by: Angus Hollands <[email protected]>

nplike → index_nplike

d5e8224

Co-authored-by: Angus Hollands <[email protected]>

nplike → index_nplike

fd8b112

Co-authored-by: Angus Hollands <[email protected]>

Choose tag_for_missing based on is_option, rather than is_record.

c226220

Co-authored-by: Angus Hollands <[email protected]>

agoose77 approved these changes Dec 1, 2022

View reviewed changes

jpivarski enabled auto-merge (squash) December 1, 2022 17:47

jpivarski merged commit 31ce657 into main Dec 1, 2022

jpivarski deleted the jpivarski/replace-simplify-method-with-classmethod-constructor branch December 1, 2022 18:01

jpivarski mentioned this pull request Jan 3, 2023

TypeError in ak.to_packed with UnionType #2058

Closed

agoose77 mentioned this pull request Jan 4, 2023

refactor: harden typetracer operations #1849

Closed

5 tasks

jpivarski removed the pr-next-release Required for the next release label Feb 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replace `simplify_optiontype` and `simplify_uniontype` with `simplified` classmethod #1928

refactor: replace `simplify_optiontype` and `simplify_uniontype` with `simplified` classmethod #1928

jpivarski commented Nov 30, 2022 •

edited by github-actions bot

Loading

codecov bot commented Nov 30, 2022 •

edited

Loading

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski commented Nov 30, 2022

agoose77 commented Nov 30, 2022

agoose77 commented Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski Nov 30, 2022

jpivarski commented Dec 1, 2022

		return continuation().simplify_optiontype()
		return continuation()

		for i, self_cont in enumerate(contents):
		for i, self_cont in enumerate(self_contents):

refactor: replace simplify_optiontype and simplify_uniontype with simplified classmethod #1928

refactor: replace simplify_optiontype and simplify_uniontype with simplified classmethod #1928

Conversation

jpivarski commented Nov 30, 2022 • edited by github-actions bot Loading

codecov bot commented Nov 30, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpivarski commented Nov 30, 2022

agoose77 commented Nov 30, 2022

agoose77 commented Nov 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpivarski commented Dec 1, 2022

refactor: replace `simplify_optiontype` and `simplify_uniontype` with `simplified` classmethod #1928

refactor: replace `simplify_optiontype` and `simplify_uniontype` with `simplified` classmethod #1928

jpivarski commented Nov 30, 2022 •

edited by github-actions bot

Loading

codecov bot commented Nov 30, 2022 •

edited

Loading