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.

Sign up for GitHub

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

Jump to bottom

refactor: from_rdataframe to use LayoutBuilder #1620

Merged
merged 9 commits into from
Aug 24, 2022
Merged

refactor: from_rdataframe to use LayoutBuilder #1620

merged 9 commits into from
Aug 24, 2022

Conversation

ianna
Copy link
Collaborator

@ianna ianna commented Aug 23, 2022 •
edited
Loading

  • Extend LayoutBuilder API to support user-allocated char buffers
  • Replace static_cast with reinterpret_cast in GrowableBuffer and LayoutBuilders
  • Cleanup some warnings
  • Replace from_rdataframe data accumulation to std::vectors with LayoutBuilders
  • Add tests

@ianna ianna marked this pull request as draft August 23, 2022 15:57
@ianna ianna changed the title refactor: use LayoutBuilder refactor: from_rdataframe to use LayoutBuilder Aug 23, 2022
@codecov
Copy link

codecov bot commented Aug 23, 2022 •
edited
Loading

Codecov Report

Merging #1620 (49ac798) into main (9e17f29) will increase coverage by 0.16%.
The diff coverage is 55.63%.

Impacted Files Coverage Δ
src/awkward/_v2/_connect/cuda/__init__.py 0.00% <0.00%> (ø)
src/awkward/_v2/_connect/numexpr.py 88.40% <0.00%> (ø)
src/awkward/_v2/_connect/pyarrow.py 88.46% <0.00%> (ø)
...awkward/_v2/_connect/rdataframe/from_rdataframe.py 0.00% <0.00%> (ø)
src/awkward/_v2/_lookup.py 98.68% <ø> (+1.17%) ⬆️
src/awkward/_v2/numba.py 93.47% <0.00%> (ø)
src/awkward/_v2/operations/ak_from_avro_file.py 66.66% <0.00%> (ø)
src/awkward/_v2/operations/ak_local_index.py 100.00% <ø> (ø)
src/awkward/_v2/operations/ak_type.py 53.84% <0.00%> (ø)
src/awkward/_v2/_connect/cling.py 24.90% <2.73%> (-1.10%) ⬇️
... and 35 more

@ianna ianna force-pushed the ianna/refactor-from_rdataframe-use-layout_builder branch from 896c56c to cbf3cbc Compare August 23, 2022 16:54
@ianna ianna force-pushed the ianna/refactor-from_rdataframe-use-layout_builder branch from dbfa633 to e4629d1 Compare August 23, 2022 20:24
@ianna ianna force-pushed the ianna/refactor-from_rdataframe-use-layout_builder branch from fea7de5 to 0d9b328 Compare August 23, 2022 20:30
ianna
ianna commented Aug 23, 2022
View reviewed changes
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 - I'm done with this PR. Please, review when you have time. Thanks!

src/awkward/_v2/_connect/rdataframe/from_rdataframe.py
ListOffsetBuilder = cppyy.gbl.awkward.LayoutBuilder.ListOffset[
"int64_t",
f"awkward::LayoutBuilder::ListOffset<int64_t, awkward::LayoutBuilder::Numpy<{data_type}>",
]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think, this should be generated from a Form. I'll do it as a separate PR after introducing multiple columns.

@ianna ianna marked this pull request as ready for review August 23, 2022 21:51
@ianna ianna requested a review from jpivarski August 23, 2022 21:51
jpivarski
jpivarski approved these changes Aug 23, 2022
View reviewed changes
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 think it looks good! I have a few questions inline about how things will evolve (will explicit functions be converted to something that works on general depth someday)?

Also, I see the to_char_buffers that we had talked about.

src/awkward/_v2/_connect/rdataframe/from_rdataframe.py Outdated Show resolved Hide resolved
src/awkward/_v2/_connect/rdataframe/from_rdataframe.py Show resolved Hide resolved
tests/v2/test_1620-layout-builders.py Show resolved Hide resolved
@jpivarski
Copy link
Member

Oh, I'll let you decide when to merge, since you might want to make changes based on my questions. But I approve the PR and you can squash-and-merge it when you're ready.

@ianna
Copy link
Collaborator Author

ianna commented Aug 24, 2022

Oh, I'll let you decide when to merge, since you might want to make changes based on my questions. But I approve the PR and you can squash-and-merge it when you're ready.

Thanks, @jpivarski! I think, I've addressed most of your comments. The recursive method is for the next PR because it has to take multiple columns to avoid multiple event loops.

@ianna ianna merged commit 9e8c841 into main Aug 24, 2022
@ianna ianna deleted the ianna/refactor-from_rdataframe-use-layout_builder branch August 24, 2022 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jpivarski jpivarski jpivarski approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ianna @jpivarski