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

fix: make C++ LayoutBuilder API consistent with the Numba's #2553

Merged
merged 13 commits into from
Jul 3, 2023

Conversation

ianna
Copy link
Collaborator

@ianna ianna commented Jun 30, 2023

LayoutBuilder in C++ and Numba should have the same API, as discussed here: #2408 (comment) by @jpivarski:

Defining the semantics

What methods should all the builders have and what should they do? (Anything that has append has an extend.) Everything has a __len__. They have the indexes they need (ListOffset has offsets, etc.) as properties.

  • Numpy: append passes a value to GrowableBuffer.append

  • Empty: has no methods; can't be filled

  • ListOffset:

    • has content property
    • begin_list returns content
    • end_list appends the length of content to offsets
  • Regular:

    • has content property
    • begin_list returns content
    • end_list does nothing
  • IndexedOption:

    • has content property
    • append_valid appends the length of content to index and returns content
    • append_invalid appends a -1 to the index and returns nothing
  • ByteMasked:

    • has content property
    • append_valid appends valid_when as a byte to mask and returns content
    • append_invalid appends not valid_when as a byte to mask and returns content
  • BitMasked:

    • has content property and a length-2 NumPy array of length and bit_position (to avoid a modulo)
    • append_valid appends valid_when as a bit to mask and updates length and bit_position and returns content
    • append_invalid appends not valid_when as a bit to mask and updates length and bit_position and returns content
  • Unmasked: has content property

  • Record:

    • has nonempty contents property
    • fields property, content(string literal) method that returns an element of the contents
  • Tuple:

    • has nonempty contents property
    • content(integer literal) method that returns an element of the contents
  • Union:

    • has contents property of length >= 2
    • append_content(integer literal) method that appends the literal integer to tags, appends the length of that one content to index and returns that one content

We're dropping List and Indexed, since they don't produce arrays that are usefully different from what you can construct with the above. This is similar to the reasoning about dropping EmptyRecord, EmptyTuple.

There are types you can't make with this: empty records and tuples, and RegularArray with size == 0. Given an array made with the above, you can get all of those with slices.

@ianna ianna marked this pull request as draft June 30, 2023 13:10
@codecov
Copy link

codecov bot commented Jun 30, 2023

Codecov Report

Merging #2553 (aa70734) into main (bb327c7) will not change coverage.
The diff coverage is n/a.

❗ Current head aa70734 differs from pull request most recent head 7b05ec7. Consider uploading reports for the commit 7b05ec7 to get more accurate results

Additional details and impacted files

@ianna ianna marked this pull request as ready for review June 30, 2023 14:09
@ianna ianna temporarily deployed to docs-preview June 30, 2023 14:25 — with GitHub Actions Inactive
@ianna
Copy link
Collaborator Author

ianna commented Jun 30, 2023

@agoose77 - Could you, please, have a look? I don't understand why the docs are not built. I see lots of warnings, but it doesn't look like they are related to this PR: https://github.com/scikit-hep/awkward/actions/runs/5423275687/jobs/9861495154 Am I wrong? Thanks!

@agoose77
Copy link
Collaborator

agoose77 commented Jun 30, 2023

@ianna the latest build reported in the PR seem to have succeeded?

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.

I recognize all of these API changes; looks good!

The C++ test has been updated—I was going to complain about tests that remove functionality that we still have, like test_List, but then realized that we retained lb::ListOffset and not lb::List. So the tests look good, too, now that I've corrected myself.

The changes to lb::Record will likely impact @ManasviGoyal's work, so she'll have to update to Awkward 2.3 soon after it gets released (maybe tomorrow).

@ianna, you can merge when ready.

@ianna ianna merged commit 628d535 into main Jul 3, 2023
35 checks passed
@ianna ianna deleted the ianna/layoutbuilder-cpp-api-update branch July 3, 2023 17:22
@ianna
Copy link
Collaborator Author

ianna commented Jul 3, 2023

Oh, I think, I broke the main build - I got a message that it needs a new C++ release :-(

@agoose77
Copy link
Collaborator

agoose77 commented Jul 3, 2023

@ianna that's OK - we anticipated it, and C++ releases conveniently can be done ahead of the Python release in an async fashion.

agoose77 added a commit that referenced this pull request Sep 27, 2023
* fix: make the examples consistent with the changes in #2553

* fix: update AWKWARD_VERSION too!

* (amend)

* fix: clarify that numpy is required

* refactor: use `find_package(Python, ...`

---------

Co-authored-by: Angus Hollands <goosey15@gmail.com>
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.

3 participants