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: handle potential TypeError when determining variable type #3516

Merged
merged 3 commits into from
Nov 4, 2023

Conversation

theOehrly
Copy link
Contributor

I ran into an issue with Seaborn 0.13.0 that wasn't present in previous versions. The following code works fine in Seaborn 0.12.2 but raises numpy.core._exceptions._UFuncInputCastingError on 0.13.0.

from datetime import timedelta
import seaborn as sns
from matplotlib import pyplot as plt

data = {
    'delta': [timedelta(1), timedelta(5), timedelta(3)],
    'x': [1, 2, 3]
}

sns.scatterplot(data, x='x', y='delta')
plt.show()

The reason are numpy's default casting rules and the fact that the check changed from np.isin(vector, [0, 1, np.nan] to np.isin(vector, [0, 1]) in the latest release. That means that previously the comparison was with 0.0 and 1.0 as float dtype, because the nan made NumPy cast the whole test array to float. Now the comparison is against int dtype.

In my opinion, the actual dtypes are not relevant here. But we should not assume that the default casting rules allow that this comparison is possible. Simply because the data type of vector is not know as the data is provided by the user.
Therefore, I'd say it is best to handle the _UFuncInputCastingError (which is a subclass of TypeError) and in that case decide that the data is not binary/boolean. The then remaining checks should not have similar issues.

Copy link

codecov bot commented Nov 4, 2023

Codecov Report

Merging #3516 (3b6e1a7) into master (8ed641f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3516   +/-   ##
=======================================
  Coverage   98.34%   98.34%           
=======================================
  Files          75       75           
  Lines       24613    24625   +12     
=======================================
+ Hits        24205    24217   +12     
  Misses        408      408           
Files Coverage Δ
seaborn/_base.py 97.38% <100.00%> (+0.01%) ⬆️
seaborn/_core/rules.py 98.41% <100.00%> (+0.07%) ⬆️
tests/_core/test_rules.py 100.00% <100.00%> (ø)
tests/test_base.py 99.70% <100.00%> (+<0.01%) ⬆️

@mwaskom mwaskom added this to the v0.13.1 milestone Nov 4, 2023
@mwaskom mwaskom changed the base branch from master to v0.13 November 4, 2023 17:16
@mwaskom mwaskom changed the base branch from v0.13 to master November 4, 2023 17:16
@mwaskom
Copy link
Owner

mwaskom commented Nov 4, 2023

Thanks @theOehrly, great catch

@mwaskom mwaskom merged commit ea51c41 into mwaskom:master Nov 4, 2023
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants