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

create test_weights.py #254

Closed
raybellwaves opened this issue Jan 31, 2021 · 6 comments · Fixed by #262
Closed

create test_weights.py #254

raybellwaves opened this issue Jan 31, 2021 · 6 comments · Fixed by #262

Comments

@raybellwaves
Copy link
Member

raybellwaves commented Jan 31, 2021

Taken from #221 (comment) (thanks @dougiesquire)

This needs to be addressed before #221.

In our testing suite we have a function https://github.com/xarray-contrib/xskillscore/blob/master/xskillscore/tests/test_deterministic.py#L72

Is there a way this could be moved upstream to conftest.py?

While verbose we could create fixtures for these (5) e.g.

adjust_weights_time():
weights.isel(lon=0, lat=0)
adjust_weights_lat():
weights.isel(lon=0, time=0)

https://matthewrocklin.com/blog/work/2019/06/23/avoid-indirection

Also i'm not sure if we are testing the weight implementation anywhere. As we found out in #248 test_determinstic.py mostly tests the ufunc part. There are some separate test files for functionality e.g. test_skipna_functionality. We should create one for weights.

@raybellwaves raybellwaves changed the title don't allow dask arrays to be mixed with numpy array (e.g. weights same dtype as observations and forecast) create test_weights Jan 31, 2021
@raybellwaves raybellwaves changed the title create test_weights create test_weights.py Jan 31, 2021
@aaronspring
Copy link
Collaborator

I like the weights function as it easily integrates in loops over dims or pytest parametrize.

My PR tested weights for some commits https://github.com/xarray-contrib/xskillscore/pull/231/files

@aaronspring
Copy link
Collaborator

I don’t understand Matthews post that we should more verbose functions here. I rather like the idea that weights is abstracted away here as the weights depend on dims. I see much code duplication in the new suggestion which will be harder to maintain.

+1 for a weights testing file refactoring.

@raybellwaves
Copy link
Member Author

I know it's a simple function. But part of me is thinking shouldn't that function be tested so I know it's doing what I expect. It appears in a couple of test files https://github.com/xarray-contrib/xskillscore/blob/master/xskillscore/tests/test_deterministic.py#L72 https://github.com/xarray-contrib/xskillscore/blob/master/xskillscore/tests/test_accessor_deterministic.py#L62 hence moving it upstream would help duplication (regardless of verbose fixture vs. function solution)

Thanks for the info in your commit. Lots of useful stuff there e.g. https://xarray.pydata.org/en/stable/generated/xarray.DataArray.weighted.html. I believe this is the file which has the weights testing https://raw.githubusercontent.com/aaronspring/xskillscore/b61185bab28f5a6d4f2aad8211e9e1881a4bb03c/asv_bench/benchmarks/xr_vs_xs.py

Some (haven't checked all) scikit-learn metrics have a weight parameter
https://scikit-learn.org/stable/modules/generated/sklearn.metrics.mean_squared_error.html#sklearn.metrics.mean_squared_error

We could create a test_weighted_metric_results_accurate.py akin to test_weighted_metric_results_accurate.py

@aaronspring
Copy link
Collaborator

Can be closed?

@raybellwaves
Copy link
Member Author

I need to finish the other (non scikit-learn metrics)

@raybellwaves
Copy link
Member Author

I need to add scipy stats then can close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants