-
Notifications
You must be signed in to change notification settings - Fork 40
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
add nan corr tests #247
add nan corr tests #247
Conversation
EDIT: Not too much related to this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am doubting the purpose of these tests now. Aren’t we essentially only testing whether the xs.metric function does the same as the np_deterministic function? Is there are reason why this should not work? Isn’t this all independent of chunking and nans?
This feedback is not too much about your changes to the code but rather what the tests check, which is not useful IMO. I would rather like to see tests checking that nans change the result for skipna True or False. Maybe this would then go into another file though...
@pytest.mark.parametrize("dim", AXES) | ||
@pytest.mark.parametrize("weight_bool", [True, False]) | ||
@pytest.mark.parametrize("skipna", [True, False]) | ||
def test_distance_metrics_xr_nan( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test could be parametrize with lazy fixture
): | ||
"""Test whether distance metrics for xarray functions can be lazy when | ||
chunked by using dask and give same results.""" | ||
chunked by using dask and give same results as np array.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the test name was misleading. We don't really check whether result is also chunked, as we often do when calling the function _dask
else: | ||
expected = metric(a.load(), b.load(), dim, _weights, skipna=skipna) | ||
assert expected.chunks is None | ||
assert_allclose(actual.compute(), expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't even need all the computes and loads. Assert triggers that on its own...
@pytest.mark.parametrize("weight_bool", [True, False]) | ||
@pytest.mark.parametrize("skipna", [True, False]) | ||
@pytest.mark.parametrize("has_nan", [True]) | ||
def test_correlation_metrics_xr_dask_nan( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try param lazy fixture
@pytest.mark.parametrize("dim", AXES) | ||
@pytest.mark.parametrize("weight_bool", [True, False]) | ||
@pytest.mark.parametrize("skipna", [True, False]) | ||
@pytest.mark.parametrize("has_nan", [True]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one here
@pytest.mark.parametrize("metrics", distance_metrics) | ||
@pytest.mark.parametrize("dim", AXES) | ||
@pytest.mark.parametrize("weight_bool", [True, False]) | ||
def test_distance_metrics_xr(a, b, dim, weight_bool, weights, metrics): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too me the test names here were all misleading. The docstring reflects what the tests do, but not the titles...
def test_correlation_metrics_xr_nan( | ||
a_nan, b_nan, dim, weight_bool, weights, metrics, skipna | ||
): | ||
"""Test whether correlation metric for xarray functions (from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am doubting the purpose of these tests now. Aren’t we essentially only testing whether the xs.metric function does the same as the np_deterministic function? Is there are reason why this should not work? Isn’t this all independent of chunking and nans?
Removed the extra tests. There's probably a bigger question about the purpose of the tests etc. but I this PR is ok for getting the correlation tests in line with the distance tests. I'm not familiar with pytest lazy fixture. |
def test_correlation_metrics_xr(a, b, dim, weight_bool, weights, metrics): | ||
@pytest.mark.parametrize("skipna", [True, False]) | ||
@pytest.mark.parametrize("has_nan", [True, False]) | ||
def test_correlation_metrics_ufunc_same_np( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how to work with lazy fixtures:
- add pytest-lazy-fixture to env
- pytest.mark.parametrize(fixtures) https://github.com/TvoroG/pytest-lazy-fixture See https://github.com/pangeo-data/climpred/blob/fdec3af0aabac42de1787aecc0fcbef64a4800f7/climpred/tests/test_bootstrap.py#L227
can be done here for this test and ufunc_dask_np
xskillscore/tests/conftest.py
Outdated
|
||
|
||
@pytest.fixture | ||
def a_nan_land(a): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we call this fixed mask and the other random nan?
if _weights is not None: | ||
_weights = _weights.load() | ||
if metric in temporal_only_metrics: | ||
expected = metric(a.load(), b.load(), dim, skipna=skipna) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compute not needed IMO
expected = metric(a.load(), b.load(), dim, skipna=skipna) | ||
else: | ||
expected = metric(a.load(), b.load(), dim, _weights, skipna=skipna) | ||
assert expected.chunks is None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that assert needed for the docstring description? IMO not
# check that chunks for chunk inputs | ||
assert actual.chunks is not None | ||
if _weights is not None: | ||
_weights = _weights.load() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compute / load now only needed because a,b,weights require being all either or not chunked
np_deterministic.py).""" | ||
if has_nan: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn’t you want to use a_nan from conftest here?
Thanks for creating #248 One thing I learned here is a we have a function that is named Couple things left. Yes I would like to use
without getting Would you mind making a code suggestion on the pytest-lazy-fixture useage: https://haacked.com/archive/2019/06/03/suggested-changes/ |
I see what you were doing. I was critizising the purpose of both of these tests. |
We either use functions inside the test to modify inputs, or we list fixtures test function arguments with pytestlazyfixture. But these different approaches shouldn't mix. I will provide a code suggestion |
@pytest.mark.parametrize("metrics", correlation_metrics) | ||
@pytest.mark.parametrize("dim", AXES) | ||
@pytest.mark.parametrize("weight_bool", [True, False]) | ||
@pytest.mark.parametrize("skipna", [True, False]) | ||
@pytest.mark.parametrize("has_nan", [True, False]) | ||
def test_correlation_metrics_daskda_same_npda( | ||
a_dask, b_dask, dim, weight_bool, weights_dask, metrics, skipna, has_nan | ||
): | ||
"""Test whether correlation metric for xarray functions can be lazy when | ||
chunked by using dask and give same results as DataArray with numpy array.""" | ||
a = a_dask.copy() | ||
b = b_dask.copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pytest.mark.parametrize("metrics", correlation_metrics) | |
@pytest.mark.parametrize("dim", AXES) | |
@pytest.mark.parametrize("weight_bool", [True, False]) | |
@pytest.mark.parametrize("skipna", [True, False]) | |
@pytest.mark.parametrize("has_nan", [True, False]) | |
def test_correlation_metrics_daskda_same_npda( | |
a_dask, b_dask, dim, weight_bool, weights_dask, metrics, skipna, has_nan | |
): | |
"""Test whether correlation metric for xarray functions can be lazy when | |
chunked by using dask and give same results as DataArray with numpy array.""" | |
a = a_dask.copy() | |
b = b_dask.copy() | |
@pytest.mark.parametrize( | |
"a2, b2", | |
[ | |
( | |
pytest.lazy_fixture("a_dask"), | |
pytest.lazy_fixture("b_dask"), | |
), | |
( | |
pytest.lazy_fixture("a"), | |
pytest.lazy_fixture("b"), | |
), | |
( | |
pytest.lazy_fixture("a_nan"), | |
pytest.lazy_fixture("b_nan"), | |
), | |
], | |
) | |
@pytest.mark.parametrize("metrics", correlation_metrics) | |
@pytest.mark.parametrize("dim", AXES) | |
@pytest.mark.parametrize("weight_bool", [True, False]) | |
@pytest.mark.parametrize("skipna", [True, False]) | |
def test_correlation_metrics_daskda_same_npda( | |
a2, b2, dim, weight_bool, weights_dask, metrics, skipna, has_nan | |
): | |
"""Test whether correlation metric for xarray functions can be lazy when | |
chunked by using dask and give same results as DataArray with numpy array.""" | |
a = a2.copy() | |
b = b2.copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requires: pip install pytest-lazy-fixture
maybe also rename a
to a_1d
so we can use a
in the parametrized tests as input. cannot give a2
and a
in this example both the same name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this example now with a_nan
is probably not better than has_nan
[True, False]. has_nan
has the advantage that the small function changes the code. on the other hand you could also add a_nan_dask
to the list of inputs, but this would be growing and growing...
there will be no 100% clean and everything matching solution in this...
I believe this PR is good to go. This closes the issue linked with a little bit of renaming to make further test development easier (or even standardized). I've opted not to use pytest-lazy-fixture but thank you for teaching me about it. I think it's very useful but as you mention I would prefer to have one method of testing per file |
Closes #246
Ignore the name of the branch. I worked on added nans to in the correlation tests.
I impact did a little bit of work on #245 but this doesn't close that.
I took out the
has_nan
parameter to the tests and instead moved to another test for nans which I could usea_nan
from the conftest.py. Unfortunately it means duplicating most of the code in the test function but from reading some pytest docs they recommend using fixtures in the conftest.py as function imputs. Not sure if we create another function inside test_deterministic or even move some upstream to conftest.py to avoid the duplicated code.