feat: made 'very optional' arguments keyword-only #1905

jpivarski · 2022-11-21T23:12:35Z

Also, the functions that are defined in terms of reducers (ak.ptp, ak.moment, ak.mean, ak.var, ak.std, ak.covar, ak.corr, ak.linear_fit, ak.softmax) now pass behavior consistently. (Previously, only ak.std made a half-hearted attempt.)

ak.to_categorical had a behavior argument added (somehow overlooked before).

The ak.*_like functions had their arguments reordered so that dtype is earlier than highlevel or behavior (which is the freedom to fix things that this PR is intended to make permanent).

In general, any ak.* function arguments that are "very optional"—configuring unusual situations—are now keyword-only, especially if they're booleans. It's very easy to mix up the order of boolean arguments. But axis arguments were left positional, since there are instances of positional axis in the wild.

The Content and Form subclasses, however, were left as they are. They're part of the low-level public API, and have only two arguments that are at all optional: parameters and nplike or form_key. These are all different types and unlikely to get misordered, and very unlikely to change order in the future. So there's not a strong need to make these arguments keyword-only.

The Type and Index subclasses are also low-level public API. Type's typestr argument and Index's metadata and nplike deserve to be keyword-only because they are rarely used and could be confusing. Any downstream developers who do use them should have a good understanding of what they are and do, at least at the level of spelling out their names.

That's everything that's going on in this PR!

📚 The documentation for this PR will be available at https://awkward-array.readthedocs.io/en/jpivarski-made-very-optional-arguments-keyword-only/ once Read the Docs has finished building 🔨

jpivarski · 2022-11-21T23:14:47Z

src/awkward/behaviors/mixins.py

-def mixin_class_method(ufunc, rhs=None, transpose=True):
+def mixin_class_method(ufunc, rhs=None, *, transpose=True):


This is one that I should ask @nsmith- about: is it the case that rhs is likely to be positional but transpose should always be written out as keyword-only?

Correct. Any binary method would need the rhs specified but transpose is more rare.
As an aside, in retrospect, I should have made transpose default false but it is too late. That was the cause of scikit-hep/coffea#674

Okay, good to know about rhs.

About transpose: changing the default from True to False would be dangerous, even at the 2.0.0 boundary, unless we also change the name. As a keyword-only argument, the written-out old name would be different from the written-out new name, so old code can't pass through accidentally. After the 2.0.0 boundary, we'd have to have a period of time in which both names exist, with a warning on the old name, but we could make a clean switch to a new name if we do it before the 2.0.0 boundary.

What do you think? Is there a better word than transpose that would justify the change in default?

I think this will just have to stay. It is not a well-trodden path anyway.

src/awkward/operations/ak_from_iter.py

ioanaif

LGTM!

jpivarski · 2022-11-22T15:44:39Z

It's weird that build-docs failed with

/home/runner/work/awkward/awkward/docs/user-guide/how-to-convert-buffers.md:54: ERROR: Unknown interpreted text role "method".
/home/runner/work/awkward/awkward/docs/user-guide/how-to-create-arraybuilder.md:379: ERROR: Unknown interpreted text role "classY".

LGTM!

Okay, great! I'm looking at the build-docs error, and if I resolve that, then I'll merge this PR. My question to @nsmith- can be followed up in a new PR, if need be.

jpivarski · 2022-11-22T15:51:47Z

There are more errors than that:

/home/runner/work/awkward/awkward/docs/reference/generated/ak.from_json.rst:24: ERROR: Unexpected indentation.
/home/runner/work/awkward/awkward/docs/reference/generated/ak.from_json.rst:143: ERROR: Unexpected indentation.
/home/runner/work/awkward/awkward/docs/reference/generated/ak.from_json.rst:185: ERROR: Unexpected indentation.
/home/runner/work/awkward/awkward/docs/reference/generated/ak.to_json.rst:13: ERROR: Unexpected indentation.
/home/runner/work/awkward/awkward/docs/reference/generated/ak.to_json.rst:25: ERROR: Unexpected indentation.
/home/runner/work/awkward/awkward/docs/user-guide/how-to-convert-buffers.md:54: ERROR: Unknown interpreted text role "method".
/home/runner/work/awkward/awkward/docs/user-guide/how-to-create-arraybuilder.md:379: ERROR: Unknown interpreted text role "classY".

The ones I've been able to trace back do seem to be legitimate errors, but I don't know why they'd stop the build now and not previously. I'll see what happens when I fix the ones I can fix.

ioanaif · 2022-11-22T15:54:56Z

There are more errors than that:

/home/runner/work/awkward/awkward/docs/reference/generated/ak.from_json.rst:24: ERROR: Unexpected indentation.
/home/runner/work/awkward/awkward/docs/reference/generated/ak.from_json.rst:143: ERROR: Unexpected indentation.
/home/runner/work/awkward/awkward/docs/reference/generated/ak.from_json.rst:185: ERROR: Unexpected indentation.
/home/runner/work/awkward/awkward/docs/reference/generated/ak.to_json.rst:13: ERROR: Unexpected indentation.
/home/runner/work/awkward/awkward/docs/reference/generated/ak.to_json.rst:25: ERROR: Unexpected indentation.
/home/runner/work/awkward/awkward/docs/user-guide/how-to-convert-buffers.md:54: ERROR: Unknown interpreted text role "method".
/home/runner/work/awkward/awkward/docs/user-guide/how-to-create-arraybuilder.md:379: ERROR: Unknown interpreted text role "classY".

The ones I've been able to trace back do seem to be legitimate errors, but I don't know why they'd stop the build now and not previously. I'll see what happens when I fix the ones I can fix.

#1904 triggers the same errors, I haven't found the cause yet.

jpivarski · 2022-11-22T15:57:32Z

The only one that I know how to fix is

ERROR: Unknown interpreted text role "classY".

It's pretty clear that this should be "class". I'll put that fix in, but it should still fail because of those other errors.

codecov · 2022-11-22T15:58:51Z

Codecov Report

Merging #1905 (1ef77ed) into main (2dbaf51) will increase coverage by 0.00%.
The diff coverage is 97.40%.

❗ Current head 1ef77ed differs from pull request most recent head bcccb47. Consider uploading reports for the commit bcccb47 to get more accurate results

Additional details and impacted files

Impacted Files	Coverage Δ
src/awkward/_util.py	`82.46% <ø> (ø)`
src/awkward/forms/bitmaskedform.py	`86.07% <ø> (ø)`
src/awkward/forms/bytemaskedform.py	`86.30% <ø> (ø)`
src/awkward/forms/emptyform.py	`87.03% <ø> (ø)`
src/awkward/forms/indexedoptionform.py	`93.15% <ø> (ø)`
src/awkward/forms/listform.py	`82.71% <ø> (ø)`
src/awkward/forms/listoffsetform.py	`93.15% <ø> (ø)`
src/awkward/forms/numpyform.py	`97.08% <ø> (ø)`
src/awkward/forms/recordform.py	`88.57% <ø> (ø)`
src/awkward/forms/regularform.py	`85.52% <ø> (ø)`
... and 95 more

agoose77 · 2022-11-22T15:59:02Z

The error here is actually due to LayoutBuilder failing in the LayoutBuilder notebook. I can take a quick look. This is actually something we should address; errors in notebooks aren't very visible in the tracebacks.

Aside - fixing the docs warnings is a long-term todo ...

agoose77 · 2022-11-22T16:05:12Z

See #1907

…rd-only

jpivarski · 2022-11-22T17:27:26Z

It's strange that #1904 now passes but #1905 still fails. Was 8d6abd6 wrong? It doesn't look like that's what the problem is.

agoose77 · 2022-11-22T17:33:58Z

@jpivarski in the docs action log, I can see mention of a different notebook this time. I'll take a look.

agoose77 · 2022-11-22T17:50:31Z

@jpivarski looks like this was a "good" failure - apparently our test suite doesn't check ak.Array.mask! Eventually mypy will catch these kinds of bugs, but it will take time to get there.

…rd-only

jpivarski · 2022-11-28T15:25:51Z

To avoid letting this get stale, I'm going to merge it now. (After a merge-with-main update and re-test.)

…rd-only

feat: made 'very optional' arguments keyword-only

13914ef

jpivarski linked an issue Nov 21, 2022 that may be closed by this pull request

Some arguments should be keyword-only. #1872

Closed

jpivarski requested a review from ioanaif November 21, 2022 23:13

jpivarski commented Nov 21, 2022

View reviewed changes

ioanaif reviewed Nov 22, 2022

View reviewed changes

src/awkward/operations/ak_from_iter.py Show resolved Hide resolved

ioanaif approved these changes Nov 22, 2022

View reviewed changes

Fix one build-docs error.

8d6abd6

Merge branch 'main' into jpivarski/made-very-optional-arguments-keywo…

94114c0

…rd-only

fix: pass keyword argument to mask

743d30c

Merge branch 'main' into jpivarski/made-very-optional-arguments-keywo…

1ef77ed

…rd-only

Merge branch 'main' into jpivarski/made-very-optional-arguments-keywo…

bcccb47

…rd-only

jpivarski enabled auto-merge (squash) November 28, 2022 15:26

jpivarski merged commit 88f7fef into main Nov 28, 2022

jpivarski deleted the jpivarski/made-very-optional-arguments-keyword-only branch November 28, 2022 15:47

jpivarski mentioned this pull request Nov 28, 2022

feat: made 'very optional' arguments keyword-only scikit-hep/uproot5#787

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: made 'very optional' arguments keyword-only #1905

feat: made 'very optional' arguments keyword-only #1905

jpivarski commented Nov 21, 2022 •

edited by github-actions bot

Loading

jpivarski Nov 21, 2022

nsmith- Nov 22, 2022 •

edited

Loading

jpivarski Nov 22, 2022

nsmith- Nov 22, 2022

ioanaif left a comment

jpivarski commented Nov 22, 2022

jpivarski commented Nov 22, 2022

ioanaif commented Nov 22, 2022

jpivarski commented Nov 22, 2022

codecov bot commented Nov 22, 2022 •

edited

Loading

agoose77 commented Nov 22, 2022 •

edited

Loading

agoose77 commented Nov 22, 2022

jpivarski commented Nov 22, 2022

agoose77 commented Nov 22, 2022

agoose77 commented Nov 22, 2022 •

edited

Loading

jpivarski commented Nov 28, 2022

		def mixin_class_method(ufunc, rhs=None, transpose=True):
		def mixin_class_method(ufunc, rhs=None, *, transpose=True):

feat: made 'very optional' arguments keyword-only #1905

feat: made 'very optional' arguments keyword-only #1905

Conversation

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

jpivarski Nov 21, 2022

Choose a reason for hiding this comment

nsmith- Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

jpivarski Nov 22, 2022

Choose a reason for hiding this comment

nsmith- Nov 22, 2022

Choose a reason for hiding this comment

ioanaif left a comment

Choose a reason for hiding this comment

jpivarski commented Nov 22, 2022

jpivarski commented Nov 22, 2022

ioanaif commented Nov 22, 2022

jpivarski commented Nov 22, 2022

codecov bot commented Nov 22, 2022 • edited Loading

Codecov Report

agoose77 commented Nov 22, 2022 • edited Loading

agoose77 commented Nov 22, 2022

jpivarski commented Nov 22, 2022

agoose77 commented Nov 22, 2022

agoose77 commented Nov 22, 2022 • edited Loading

jpivarski commented Nov 28, 2022

jpivarski commented Nov 21, 2022 •

edited by github-actions bot

Loading

nsmith- Nov 22, 2022 •

edited

Loading

codecov bot commented Nov 22, 2022 •

edited

Loading

agoose77 commented Nov 22, 2022 •

edited

Loading

agoose77 commented Nov 22, 2022 •

edited

Loading