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

CI: Remove float16 fixture value #32221

Merged

Conversation

SaturnFromTitan
Copy link
Contributor

@SaturnFromTitan SaturnFromTitan commented Feb 24, 2020

Hopefully, this fixes the flaky test

@SaturnFromTitan SaturnFromTitan changed the title removing float16 fixture value as it should not be supported CI: Remove float16 fixture value Feb 24, 2020
@WillAyd
Copy link
Member

WillAyd commented Feb 25, 2020

@jreback any reason you know of we should keep this fixture value?

@WillAyd WillAyd added the Unreliable Test Unit tests that occasionally fail label Feb 25, 2020
@jreback
Copy link
Contributor

jreback commented Feb 25, 2020

we very little support for float16; it does work in places but if it’s causing issues then maybe should just xfail those

@SaturnFromTitan SaturnFromTitan force-pushed the remove-float16-fixture-value branch from d5a860e to 12ef51e Compare February 25, 2020 12:12
@SaturnFromTitan
Copy link
Contributor Author

If we don't support it, then removing it from the fixture values seems more maintainbale than adding a lot of xfail(strict=False) @jreback

@simonjayhawkins
Copy link
Member

If we don't support it, then removing it from the fixture values seems more maintainbale than adding a lot of xfail(strict=False) @jreback

It appears that the test design is flawed. so we should change the test. Looking at the differences -0.68505859375 appears once in the result and twice in the expected. The result is correct. The test assumes that the randomly generated numbers in the Series will be unique. This is not always the case.

Series length are different
E       [left]:  28, Float64Index([  -0.355712890625,    -0.71435546875,     0.39794921875,
E                      -0.1519775390625,       -1.14453125,       1.181640625,
E                         -1.3037109375,        2.13671875,      -0.751953125,
E                           2.005859375,   -0.399169921875, -0.06842041015625,
E                        -0.68505859375,        1.07421875,      0.6845703125,
E                       -0.133544921875,      1.6826171875,     0.41357421875,
E                        -0.03857421875,          1.015625,      0.2802734375,
E                       -0.477294921875,  -0.1910400390625,     0.33544921875,
E                         0.59423828125,     -0.9111328125,   -0.098388671875,
E                                   nan],
E                    dtype='float64')
E       [right]: 29, Float64Index([  -0.355712890625,    -0.71435546875,     0.39794921875,
E                      -0.1519775390625,       -1.14453125,       1.181640625,
E                         -1.3037109375,        2.13671875,      -0.751953125,
E                           2.005859375,   -0.399169921875, -0.06842041015625,
E                            1.07421875,      0.6845703125,   -0.133544921875,
E                          1.6826171875,    -0.68505859375,     0.41357421875,
E                        -0.03857421875,          1.015625,      0.2802734375,
E                       -0.477294921875,  -0.1910400390625,     0.33544921875,
E                         0.59423828125,    -0.68505859375,     -0.9111328125,
E                       -0.098388671875,               nan],
E                    dtype='float64')

@simonjayhawkins
Copy link
Member

If we don't support it, then removing it from the fixture values seems more maintainbale than adding a lot of xfail(strict=False) @jreback

It appears that the test design is flawed. so we should change the test.

but to get ci to green quickly for now could just...

diff --git a/pandas/tests/base/test_ops.py b/pandas/tests/base/test_ops.py
index 625d55900..54e459ed2 100644
--- a/pandas/tests/base/test_ops.py
+++ b/pandas/tests/base/test_ops.py
@@ -277,6 +277,8 @@ class TestIndexOps(Ops):
             pytest.skip(f"values of {klass} cannot be changed")
         elif isinstance(orig, pd.MultiIndex):
             pytest.skip("MultiIndex doesn't support isna")
+        elif not orig.is_unique:
+            pytest.xfail("FIXME: test assumes fixture has unique values gh-32220")
 
         # special assign to the numpy array
         if is_datetime64tz_dtype(obj):

and not mark the issue as closed.

@SaturnFromTitan
Copy link
Contributor Author

@simonjayhawkins Ah ok, I got confused because the issue mentioned test_value_counts_unique_nunique failing and that one already has an xfail for duplicated values.

However, you're right and test_value_counts_unique_nunique_null is actually failing. I think adding the xfail now and refactoring the tests later is the best approach 👍

@SaturnFromTitan
Copy link
Contributor Author

Changed the PR to use the same xfail message as test_value_counts_unique_nunique. Both share the same flawed implementation

@simonjayhawkins
Copy link
Member

Changed the PR to use the same xfail message as test_value_counts_unique_nunique. Both share the same flawed implementation

makes sense and we should leave the issue open.

@simonjayhawkins simonjayhawkins added the CI Continuous Integration label Feb 25, 2020
@simonjayhawkins simonjayhawkins modified the milestones: 1.0.2, 1.1 Feb 25, 2020
@simonjayhawkins
Copy link
Member

no need to backport as #32046 was not backported.

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

ci is green ex. conda issue.

@simonjayhawkins
Copy link
Member

since #32241 is merged, merging this to get ci to green.

@simonjayhawkins simonjayhawkins merged commit e88629f into pandas-dev:master Feb 25, 2020
@simonjayhawkins
Copy link
Member

Thanks @SaturnFromTitan

roberthdevries pushed a commit to roberthdevries/pandas that referenced this pull request Mar 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration Unreliable Test Unit tests that occasionally fail
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants