awkward icon indicating copy to clipboard operation
awkward copied to clipboard

refactor: prepare to deprecate camelCase `toRegularArray` et al.

Open agoose77 opened this issue 2 years ago • 8 comments

This PR introduces snake_case methods for toRegularArray et al. This means in future we can introduce a PR to deprecate these.

agoose77 avatar Sep 06 '22 10:09 agoose77

Codecov Report

Merging #1676 (f3b3670) into main (e692946) will increase coverage by 0.32%. The diff coverage is 83.78%.

Additional details and impacted files
Impacted Files Coverage Δ
src/awkward/_v2/_connect/jax/__init__.py 90.47% <ø> (+1.58%) :arrow_up:
src/awkward/_v2/operations/ak_broadcast_arrays.py 100.00% <ø> (+4.34%) :arrow_up:
src/awkward/_v2/operations/ak_transform.py 65.51% <ø> (+56.89%) :arrow_up:
src/awkward/_v2/types/uniontype.py 84.61% <ø> (ø)
src/awkward/_v2/highlevel.py 71.68% <33.33%> (-0.16%) :arrow_down:
src/awkward/_v2/_util.py 83.79% <50.00%> (ø)
src/awkward/_v2/contents/indexedarray.py 75.58% <54.54%> (+1.33%) :arrow_up:
src/awkward/_v2/contents/bitmaskedarray.py 71.27% <58.82%> (-0.26%) :arrow_down:
src/awkward/_v2/behaviors/categorical.py 73.13% <60.00%> (-8.94%) :arrow_down:
src/awkward/_v2/contents/unmaskedarray.py 70.58% <61.53%> (-0.30%) :arrow_down:
... and 57 more

codecov[bot] avatar Sep 06 '22 10:09 codecov[bot]

The toto_ is fine, but the RegularArray names the type it's converting it into. Wouldn't the function of to_RegularArray be clearer than to_regular_array?

jpivarski avatar Sep 06 '22 17:09 jpivarski

The toto_ is fine, but the RegularArray names the type it's converting it into. Wouldn't the function of to_RegularArray be clearer than to_regular_array?

Personally, I don't like mixed cases, which motivated this PR in the first place. I don't find the name unclear, after all we have to_arrow instead of to_Table (or whichever type we convert to).

agoose77 avatar Sep 06 '22 19:09 agoose77

Okay, sure. :)

jpivarski avatar Sep 06 '22 22:09 jpivarski

Also, I don't think you got all of the option-type conversion methods:

  • toBitMaskedArray
  • toByteMaskedArray
  • toIndexedOptionArray Because of these questions, I'm going to hold off on this at least until after 1.10.0rc3.

Ah, that's what I was trying to remember during the meeting. I meant to add these as todo-items following discussion :face_in_clouds:

agoose77 avatar Sep 09 '22 07:09 agoose77

No deprecation period is needed before the 2.0.0 release.

jpivarski avatar Sep 09 '22 13:09 jpivarski

Oh, right - we're not guaranteeing that the layout API is even remotely backwards compatible with v1. I keep forgetting this fact, because I think of layouts as public API (albeit a low level one)

agoose77 avatar Sep 09 '22 13:09 agoose77

That's right, the layouts are a low-level, public API, and we've advertised that we will be changing these slightly. Slightly means some methods/properties will have different names, which is what this PR is doing. It's the high-level, public API that's not changing (much? at all?).

One of the earlier changes we put into v2 was to change low-level, public API method names so that they would agree with their corresponding high-level, public API names. So this sort of thing is fair game.

jpivarski avatar Sep 09 '22 13:09 jpivarski

This will need rebasing, so I'll close this for now.

agoose77 avatar Sep 28 '22 10:09 agoose77