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

ArrayBuilder refactoring #977

Merged
merged 18 commits into from
Aug 3, 2021
Merged

ArrayBuilder refactoring #977

merged 18 commits into from
Aug 3, 2021

Conversation

ianna
Copy link
Collaborator

@ianna ianna commented Jul 2, 2021

  • move all *::snapshot methods that depended on Content classes to a builder_snapshot(const ak::Builder& builder) function invoked in content.cpp to be handled via pybind11
    - [ ] move ArrayBuilder::append methods to be handled via pybind11
    as discussed, this one is needed for numba and will be revisited later
  • remove RawArray class
  • remove *.cpp tests
  • remove dependent project

@ianna ianna marked this pull request as draft July 2, 2021 09:48
Copy link
Collaborator Author

@ianna ianna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jpivarski - please, check if it's going in the right direction. Thanks.

src/libawkward/builder/ArrayBuilder.cpp Outdated Show resolved Hide resolved
src/libawkward/builder/ArrayBuilder.cpp Outdated Show resolved Hide resolved
src/libawkward/builder/ArrayBuilder.cpp Show resolved Hide resolved
src/libawkward/io/json.cpp Show resolved Hide resolved
@ianna ianna force-pushed the ianna/array-builder-refactoring branch from 9716a3e to 0b498db Compare July 30, 2021 07:17
@ianna ianna changed the title start on ArrayBuilder refactoring ArrayBuilder refactoring Jul 30, 2021
@ianna ianna marked this pull request as ready for review August 3, 2021 16:02
@ianna ianna requested a review from jpivarski August 3, 2021 16:02
@ianna
Copy link
Collaborator Author

ianna commented Aug 3, 2021

@jpivarski - the last commit was a minor cleanup, so I assume the tests will pass. Please, have a look when you have time. Thanks!

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.

Awesome! I scanned through everything and marvel at how many things used to refer to Content but didn't really need to (including in the documentation).

About deleting "dependent project," I had thought that we'd just disable it, but I like your idea of actually deleting it. It's best to be clear that the capability is going away.

I also like the idea of concentrating transitional code in include/awkward/refactoring.h, so that we can find it again when the transition is complete.

Let's merge it! You can do the honors (pushing the "enable squash-and-merge" button), in case you have any last changes.

@ianna ianna merged commit def6bd1 into main Aug 3, 2021
@ianna ianna deleted the ianna/array-builder-refactoring branch August 3, 2021 17:28
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