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

Update tests for matplotlib 3.8.0rc1 #3446

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Conversation

ksunden
Copy link
Contributor

@ksunden ksunden commented Aug 24, 2023

This is mostly a generalization of the tests to work with both old and new matplotlib

The only production code change is adding .flat to one line in the matrix annotation code, all other changes are test-only.

Mostly the test changes were allowing the collection being found to be a QuadContourSet, as well as allowing outputs like colors or coordinates to be sequences instead of singular values, or adding .flat to values which now get returned as input-shaped arrays instead of automatically flattened arrays.

I was able to avoid version checking for most things, aside from the minor tick expected value and explicitly checking the expected type in ax.collections. Though there are some version-checks by proxy of isinstance checks.

I did add a boolean flag to shift the filtering of empty paths from the test to the helper function for get_contour_coords, this is only used in one test for now, but made for a backwards compatible filtering step.

get_contour_coords will now return non-rectangular grids (Not actually clear to me that this is new) so some tests have additional loops before calling assert_array_equal I added assertions that the lengths were the same to avoid zip dropping the fact that one list was longer.

I have tested with both matplotlib v3.8.x (the backport branch for the 3.8 series) as well as released 3.7.2.

In my dev environment I am seing some numpy warnings (which are elevated to errors for 2 tests), but other than that all tests pass.

@codecov
Copy link

codecov bot commented Aug 24, 2023

Codecov Report

Merging #3446 (779402f) into master (67a777a) will decrease coverage by 0.01%.
The diff coverage is 97.36%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3446      +/-   ##
==========================================
- Coverage   98.32%   98.32%   -0.01%     
==========================================
  Files          77       77              
  Lines       24403    24413      +10     
==========================================
+ Hits        23995    24004       +9     
- Misses        408      409       +1     
Files Changed Coverage Δ
seaborn/matrix.py 96.18% <ø> (ø)
tests/test_distributions.py 99.73% <97.14%> (-0.07%) ⬇️
tests/_core/test_scales.py 100.00% <100.00%> (ø)

@mwaskom
Copy link
Owner

mwaskom commented Aug 24, 2023

Thanks @ksunden for jumping in here with the quick help — I can confirm that this passes all test using the rc on my laptop too!

@mwaskom mwaskom merged commit b3e6b8d into mwaskom:master Aug 24, 2023
9 of 10 checks passed
@mwaskom
Copy link
Owner

mwaskom commented Aug 25, 2023

Ah dang, not sure these test updates are all correct. There was a (sometimes triggered) accession of the contours.collections inside seaborn's main codebase that was populating the collections list as previously expected. So the filter_empty addition allowed tests to pass using that data, but not once I fixed the deprecated (as opposed to broken) pattern :(

mwaskom added a commit that referenced this pull request Aug 25, 2023
@ksunden
Copy link
Contributor Author

ksunden commented Aug 25, 2023

Ah, darn, that was the trickiest bit that I was not quite as confident of (but it appeared to work for both old and new versions of mpl, so I went with it).

If I understand what's happening, the following patch should work:

diff --git a/tests/test_distributions.py b/tests/test_distributions.py
index 3b8418fb..54de2b04 100644
--- a/tests/test_distributions.py
+++ b/tests/test_distributions.py
@@ -2438,13 +2438,13 @@ class TestDisPlot:
         z = [0] * 80 + [1] * 20

         g = displot(x=x, y=y, col=z, kind="kde", levels=10)
-        l1 = sum(bool(get_contour_coords(c)) for c in g.axes.flat[0].collections)
-        l2 = sum(bool(get_contour_coords(c)) for c in g.axes.flat[1].collections)
+        l1 = sum(len(get_contour_coords(c, True)) for c in g.axes.flat[0].collections)
+        l2 = sum(len(get_contour_coords(c, True)) for c in g.axes.flat[1].collections)
         assert l1 > l2

         g = displot(x=x, y=y, col=z, kind="kde", levels=10, common_norm=False)
-        l1 = sum(bool(get_contour_coords(c)) for c in g.axes.flat[0].collections)
-        l2 = sum(bool(get_contour_coords(c)) for c in g.axes.flat[1].collections)
+        l1 = sum(len(get_contour_coords(c, True)) for c in g.axes.flat[0].collections)
+        l2 = sum(len(get_contour_coords(c, True)) for c in g.axes.flat[1].collections)
         assert l1 == l2

     def test_bivariate_hist_norm(self, rng):

i.e. using the filter_empty behavior for one additional test, and changing from bool to len, as there may be more than one now

(I just commented out the line which accessed cset.collections, so whatever the proper fix on that side is up to you :))

mwaskom added a commit that referenced this pull request Aug 25, 2023
* Add explicit observed=False to groupby calls per upcoming pandas change

* Remove deprecated approach to setting contour label, which never actually worked

* Remove test case that raises seaborn deprecation warning

* Silence pandas deprecation warning

* Avoid pandas warning for setting nan on int serires

* Addressed contour API change missed in #3446

* Only cast integer series to floats before adding nulls

* Avoid pandas SettingWithCopyWarning

* Address another case of setting nan on int series

* Handle upcoming deprecation of pd.unique(list)

* Avoid casting nonfinite values to intp while property mapping

* Ignore numpy warning when all violins are singular

* Catch another invalid cast

* Ignore persistent xdist warning from test machinery

* Don't convert warnigns to errors in test config
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