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(rust,python): Fix edge-case where the Array dtype could (internally) be considered numeric #11398

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

alexander-beedie
Copy link
Collaborator

@alexander-beedie alexander-beedie commented Sep 28, 2023

Came across this one after seeing some inconsistent error messages debugging something else.
(Addresses an edge-case where Array dtype could inadvertantly be considered numeric).

  • The is_numeric check was a bit peculiar (and likely prone to future breakage if/when adding new dtypes) as it decided that a dtype was numeric if It wasn't various other dtypes, rather than because it was numeric. Changed this so that we positively assert the dtype from the known numeric types instead, which seems a lot cleaner.

Also: fixes a minor test lint (mypy on py 3.11) caused by an update to pandas-stubs.

Note: for some reason we don't currently consider Decimal to be numeric here; that seems odd, so will probably revisit to better-understand why... this PR does not change this behaviour (indeed, quite a few things blow up if you do ;)

@github-actions github-actions bot added fix Bug fix python Related to Python Polars rust Related to Rust Polars labels Sep 28, 2023
@alexander-beedie alexander-beedie force-pushed the array-dtype-not-numeric branch 6 times, most recently from fa67742 to 18781a5 Compare September 28, 2023 20:46
Copy link
Member

@stinodego stinodego left a comment

Choose a reason for hiding this comment

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

I was actually staring at that is_numeric implementation yesterday... I'd say your implementation is much better 👌

for some reason we don't currently consider Decimal to be numeric

Ritchie explained this to me yesterday (I had also noticed this). It's not numeric because you cannot do regular arithmetic with it.

I'm always happy with more pytest.mark.parametrize 😄 I added a small commit to add the correct type hint and remove the warning filter (apparently it was unnecessary).

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Sep 29, 2023

Ritchie explained this to me yesterday (I had also noticed this). It's not numeric because you cannot do regular arithmetic with it.

I figured it would be something like that - good to know :)
Maybe we can add a more inclusive numeric method for other uses later...

I'm always happy with more pytest.mark.parametrize 😄 I added a small commit to add the correct type hint and remove the warning filter (apparently it was unnecessary).

Ahh, well spotted!

@stinodego stinodego merged commit 400cf12 into pola-rs:main Sep 29, 2023
24 checks passed
@stinodego
Copy link
Member

Maybe we can add a more inclusive numeric method for other uses later...

Yeah this begs the question 'what exactly is numeric' and maybe we should be calling it differently... not super simple to answer though. I decided to leave it alone for now.

@alexander-beedie alexander-beedie deleted the array-dtype-not-numeric branch September 29, 2023 11:18
romanovacca pushed a commit to romanovacca/polars that referenced this pull request Oct 1, 2023
…ly) be considered numeric (pola-rs#11398)

Co-authored-by: Stijn de Gooijer <stijn@degooijer.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Bug fix python Related to Python Polars rust Related to Rust Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants