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

Remove duplicated import of to/from-parquet #1435

Merged
merged 2 commits into from
Apr 20, 2022

Conversation

douglasdavis
Copy link
Contributor

I noticed when using the new from_parquet (implemented in awkward._v2.operations.convert) that the top level awkward._v2 namespace imports the parquet read/write functions twice.

@douglasdavis
Copy link
Contributor Author

Perhaps this PR should also remove the ak_from_parquet.py and ak_to_parquet.py files in the io module?

@jpivarski
Copy link
Member

Perhaps this PR should also remove the ak_from_parquet.py and ak_to_parquet.py files in the io module?

Yes, that would be helpful, thank you! This PR illustrates the fact that the "io" submodule has to be removed because I forgot and created from_parquet/to_parquet in two places. When I revamp the from_json/to_json, I'll move them to "convert" also.

(There's not enough mental distinction between "io" and "convert" to be clear on which should go where. Users don't see it, anyway—everything is just ak.*—it was just for our internal organization. Maybe the whole level of hierarchy should be reduced by one, so that all of those ak_* submodules are directly in "operations". Almost everything is in "structure", anyway. I may do that at the same time as internal renaming, to get the layout method names consistent with the high-level function names, but that would be a separate PR, timed to not interfere with other work.)

@codecov
Copy link

codecov bot commented Apr 20, 2022

Codecov Report

Merging #1435 (518397c) into main (edfce38) will decrease coverage by 0.18%.
The diff coverage is 58.57%.

Impacted Files Coverage Δ
src/awkward/_v2/_connect/cuda/__init__.py 0.00% <0.00%> (ø)
src/awkward/_v2/_connect/jax/__init__.py 0.00% <0.00%> (ø)
src/awkward/_v2/operations/convert/ak_from_cupy.py 50.00% <0.00%> (+23.33%) ⬆️
src/awkward/_v2/operations/convert/ak_from_iter.py 93.75% <ø> (ø)
src/awkward/_v2/operations/convert/ak_from_jax.py 50.00% <0.00%> (-25.00%) ⬇️
...wkward/_v2/operations/convert/ak_to_arrow_table.py 100.00% <ø> (ø)
src/awkward/_v2/operations/convert/ak_to_cupy.py 33.33% <0.00%> (+23.95%) ⬆️
src/awkward/_v2/operations/convert/ak_to_jax.py 33.33% <0.00%> (-41.67%) ⬇️
src/awkward/_v2/operations/describe/ak_backend.py 10.00% <0.00%> (-2.50%) ⬇️
src/awkward/_v2/operations/io/__init__.py 100.00% <ø> (ø)
... and 67 more

@douglasdavis
Copy link
Contributor Author

Sounds good! this narrow focused PR should be good to go then

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.

Thanks!

@jpivarski jpivarski enabled auto-merge (squash) April 20, 2022 18:00
@jpivarski jpivarski merged commit ec9eec9 into scikit-hep:main Apr 20, 2022
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