Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ refactoring: ak.type and ak.values_astype #1234

Merged
merged 18 commits into from
Jan 21, 2022
Merged

Conversation

ioanaif
Copy link
Collaborator

@ioanaif ioanaif commented Jan 19, 2022

No description provided.

@ioanaif ioanaif marked this pull request as draft January 19, 2022 16:50
@codecov
Copy link

codecov bot commented Jan 19, 2022

Codecov Report

Merging #1234 (5816e05) into main (0a0e9be) will decrease coverage by 0.49%.
The diff coverage is 78.51%.

Impacted Files Coverage Δ
src/awkward/_v2/contents/bitmaskedarray.py 64.45% <ø> (-0.79%) ⬇️
src/awkward/_v2/contents/bytemaskedarray.py 82.81% <ø> (+3.66%) ⬆️
src/awkward/_v2/contents/emptyarray.py 69.54% <ø> (ø)
src/awkward/_v2/contents/indexedarray.py 60.74% <ø> (+0.98%) ⬆️
src/awkward/_v2/contents/content.py 81.57% <28.57%> (-1.62%) ⬇️
src/awkward/_v2/_typetracer.py 68.94% <33.33%> (-0.25%) ⬇️
src/awkward/_v2/operations/describe/ak_type.py 45.71% <38.70%> (-34.29%) ⬇️
src/awkward/_v2/highlevel.py 65.59% <60.00%> (+0.60%) ⬆️
...c/awkward/_v2/operations/reducers/ak_linear_fit.py 85.71% <66.66%> (-1.52%) ⬇️
src/awkward/_v2/operations/reducers/ak_all.py 91.66% <75.00%> (-8.34%) ⬇️
... and 50 more

@ioanaif ioanaif marked this pull request as ready for review January 20, 2022 17:22
Copy link
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.

On the whole, it's looking good, but I have a few requests. Most of them are about using centralized utilities, reducing the proliferation of type-lists that were in v1.

The type name for leaves of the type tree changed from PrimitiveType to NumpyType. As far as I can see, there is no ak._v2.types.PrimitiveType, so how are these tests working? Virtually every attempt to build a type would have to terminate on these leaf-nodes, so you ought to be getting

AttributeError: module 'awkward._v2.types' has no attribute 'PrimitiveType'

(I don't see a PrimitiveType being defined in this diff.)

Anyway, in an attempt to use terminology more consistently, the NumpyArray has NumpyForm and NumpyType. We still use the word "primitive," but it refers to the string naming the type in Forms. v1 and v2 have to be able to accept each others' Forms so that pickled data from each version can be used in the other (that was a goal, so that users could build work-arounds if they need to), and that means the JSON field name "primitive" in the Form can't be changed. (Otherwise, a better name might be "dtype", though that would also be misleading because dtypes are Python objects, and this is a string. So it's just going to be "primitive".)

src/awkward/_v2/contents/numpyarray.py Outdated Show resolved Hide resolved
src/awkward/_v2/operations/describe/ak_type.py Outdated Show resolved Hide resolved
src/awkward/_v2/operations/describe/ak_type.py Outdated Show resolved Hide resolved
src/awkward/_v2/operations/describe/ak_type.py Outdated Show resolved Hide resolved
src/awkward/_v2/operations/structure/ak_values_astype.py Outdated Show resolved Hide resolved
@jpivarski jpivarski enabled auto-merge (squash) January 21, 2022 17:20
@jpivarski jpivarski merged commit 3af8b01 into main Jan 21, 2022
@jpivarski jpivarski deleted the ioanaif/ak.values_astype branch January 21, 2022 17:53
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.

2 participants