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

Improving Python DataType support for Struct and repr #3471

Merged
merged 2 commits into from
May 23, 2022

Conversation

cjermain
Copy link
Contributor

While working on #3413, I noticed that the Python representation of DataTypes has some issues. This PR fixes a few of them:

  • Struct dtype could not be converted back to DataType::Struct, since previously it had an incomplete representation
  • dtype_string_repr was not implemented yet, so it always failed to import

The __repr__ behavior is a bit odd, but much closer in this PR to the Rust API. This is because the class is being used as a singleton without being constructed -- type(dtype) == type. So for native types repr(dtype) is actually type(dtype).__repr__(dtype) which gives strings like "<class 'polars.datatypes.Utf8'>". I left the DataType.string_repr method, but made it more clear that as a classmethod this uses cls not self. I think this could be improved by having an instantiated singleton that the class provides on the new constructor. This can be a follow up.

@github-actions github-actions bot added python Related to Python Polars rust Related to Rust Polars labels May 22, 2022
@cjermain
Copy link
Contributor Author

I'm not seeing the tests fail locally. I'm wondering if the test_arr_unique test is intermittent:
https://github.com/pola-rs/polars/runs/6543827625?check_suite_focus=true#step:9:514

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2022

Codecov Report

Merging #3471 (d45d6d6) into master (f14570a) will decrease coverage by 0.00%.
The diff coverage is 62.96%.

@@            Coverage Diff             @@
##           master    #3471      +/-   ##
==========================================
- Coverage   77.61%   77.61%   -0.01%     
==========================================
  Files         408      408              
  Lines       69847    69876      +29     
==========================================
+ Hits        54214    54234      +20     
- Misses      15633    15642       +9     
Impacted Files Coverage Δ
py-polars/polars/__init__.py 100.00% <ø> (ø)
py-polars/src/lib.rs 93.51% <16.66%> (-1.27%) ⬇️
py-polars/src/conversion.rs 78.27% <64.28%> (-2.72%) ⬇️
py-polars/polars/datatypes.py 92.00% <72.22%> (-2.65%) ⬇️
polars/polars-core/src/datatypes.rs 69.57% <100.00%> (-0.20%) ⬇️
...olars/polars-core/src/frame/groupby/into_groups.rs 54.21% <0.00%> (-0.37%) ⬇️
...ars/polars-core/src/chunked_array/ops/aggregate.rs 94.30% <0.00%> (+0.09%) ⬆️
polars/polars-core/src/series/mod.rs 57.85% <0.00%> (+0.13%) ⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f14570a...d45d6d6. Read the comment docs.

@ritchie46
Copy link
Member

Could you do a rebase on master? That should fix it.

@ritchie46
Copy link
Member

Thanks for this. I agree it's something we can improve because of the nested dtypes.

@ritchie46 ritchie46 merged commit c6d11d9 into pola-rs:master May 23, 2022
moritzwilksch pushed a commit to moritzwilksch/polars that referenced this pull request May 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants