add deterministic xr-metrics to asv benchmark and asv refactor

[aaronspring]

Description

• added deterministic metrics only using xarray and not numpy. For small data, this is faster:

The good news is that xskillscore beats xr-metrics for large inputs, at least on my laptop, 10-40%.

The distance metrics have also the keywords skipna and weighted and are much more concise (6 lines of code only).
An xarray PR for xr.corr(weighted, skipna) would be nice.

• refactored asv benchmarks

Type of change

Please delete options that are not relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Performance (if you touched existing code run asv to detect performance changes)
• refactoring

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. This could point to a cell in the updated notebooks. Or a snippet of code with accompanying figures here.

Checklist (while developing)

• I have added docstrings to all new functions.
• I have commented my code, particularly in hard-to-understand areas

Pre-Merge Checklist (final steps)

• I have rebased onto master or develop (wherever I am merging) and dealt with any conflicts.
• I have squashed commits to a reasonable amount, and force-pushed the squashed commits.

References

Please add any references to manuscripts, textbooks, etc.

[raybellwaves]

I'm fine with this going in. I'm also ok with not documenting it. Just add a note in the CHANGELOG about it being for advanced users.

[raybellwaves]

Just update the CHANGELOG and i'll merge if you think this is good to go.

[ahuang11]

How much faster is this for smaller datasets compared to the original np_deterministic? I am afraid of redundancy and the resulting maintenance required if you do add these. Not to mention, for new users, if they see two methods for RMSE, they may be confused as to why they would choose one over the other (e.g. pandas and their redundant methods like pd.read_table, pd.read_csv, etc)

[raybellwaves]

@ahuang11 raises good points.

@aaronspring If you do sphinx-autogen -o api api.rst. Do they get added? I'd prefer for them not too. But you haven't committed any docs so i'm still ok with it.

That said I don't think we have the user base of pandas :) and there are learnings from the benchmark. To Andrew's point I don't think it's worth maintaining and leave it here for advanced users.

[ahuang11]

Sorry, I have trouble deciphering the benchmark. I think I only see the after, but not the before. How much faster is it?

Nevermind I see it now
xr_mse vs mse. If it's only a couple of seconds I don't think it's worth the divergence.

From the zen of python:
https://www.python.org/dev/peps/pep-0020/
"There should be one-- and preferably only one --obvious way to do it."

And less is more.
https://www.oreilly.com/library/view/becoming-a-better/9781491905562/ch04.html

In a similar sense, if you compare numpy vs the built-in math module, math is faster for small datasets as well, but numpy does not implement two methods for np.sum.
https://stackoverflow.com/questions/3650194/are-numpys-math-functions-faster-than-pythons

[aaronspring]

@aaronspring If you do sphinx-autogen -o api api.rst. Do they get added? I'd prefer for them not too. But you haven't committed any docs so i'm still ok with it.

the dont get added, because I didnt add them to api.rst

That said I don't think we have the user base of pandas :) and there are learnings from the benchmark. To Andrew's point I don't think it's worth maintaining and leave it here for advanced users.

all these functions are not available for xs.metric only by xs.xr.metric.

[aaronspring]

How much faster is this for smaller datasets compared to the original np_deterministic? I am afraid of redundancy and the resulting maintenance required if you do add these.

I agree my new code is redundant for the user (unless interested in trivial speedups in the milliseconds).

Not to mention, for new users, if they see two methods for RMSE, they may be confused as to why they would choose one over the other (e.g. pandas and their redundant methods like pd.read_table, pd.read_csv, etc)

no users wont see this, as it is not in the main API xs.metric but xs.xr.metric.

These functions provide:
a) a test against xarray that they return correct results (see it as an independent test of weighted)
b) a time/peakmem benchmark comparing against xarray

I am not arguing that we should maintain this code. I think the distance based xr metrics wont require any maintinance because they are written so simple. and if they break somehow, we can also get rid of them.

[raybellwaves]

You may have to manually update (rebase) CHANGELOG https://github.com/xarray-contrib/xskillscore/blob/master/CHANGELOG.rst

[aaronspring]

I added a small disclaimer to the new files.

While I see the risk from this redundancy, I also see the gain from independent testing via xarray based functions.

I leave it up to you guys whether you think this PR is useful.

If not, I can also just delete the xr metrics part, and just merge the asv refactoring. I dont have high stakes in this.

[ahuang11]

Since it's only a few seconds faster, I would say don't merge it. If it's about testing, you could add to the unit testing. But I also don't have high stakes.

…

On Tue, Jan 12, 2021, 8:36 AM Aaron Spring ***@***.***> wrote: I added a small disclaimer to the new files. While I see the risk from this redundancy, I also see the gain from independent testing via xarray based functions. I leave it up to you guys whether you think this PR is useful. If not, I can also just delete the xr metrics part, and just merge the asv refactoring. I dont have high stakes in this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#231 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADU7FFQUHEXK4JGUBDGWCC3SZRM7PANCNFSM4VVQAISA> .

[raybellwaves]

I'm leaning towards keeping the asv stuff

[aaronspring]

20-30% is often worth optimizing. And those numbers actually show how xskillscore is faster than just using xarray when relevant (big data).

[aaronspring]

do you think implementing weights and skipna in to xr.corr would be useful? see my issue pydata/xarray#4768

[aaronspring]

removed xs.xr, first deleted also the benckmark comparison with xr metrics and then took in again. how should I get rid of this as well?

[ahuang11]

I would agree about the 20-30% optimization in speed if the time scale was on minutes / hours and it was able to outscale its counterpart.

For example, from https://stackoverflow.com/questions/3650194/are-numpys-math-functions-faster-than-pythons

lebigot@weinberg ~ % python -m timeit 'abs(3.15)' 
10000000 loops, best of 3: 0.146 usec per loop

lebigot@weinberg ~ % python -m timeit -s 'from numpy import abs as nabs' 'nabs(3.15)'
100000 loops, best of 3: 3.92 usec per loop

abs is 26x faster than numpy.abs, but it does not warrant an additional implementation in numpy because the time scale is in microseconds, and numpy outscales the std lib math.

[aaronspring]

create a 1e4x1e4x1e4 array, and you longer compute times

[raybellwaves]

@aaronspring Is this good to go?

Sorry I don't get the question here

removed xs.xr, first deleted also the benckmark comparison with xr metrics and then took in again. how should I get rid of this as well?

[raybellwaves]

Thanks @aaronspring