better bootstrapping #285
better bootstrapping #285
Conversation
|
performance increase on my 2-core macbookpro 2018: This 9x performance increase comes from removing the unnecessary |
climpred/tests/test_bootstrap.py
Outdated
| @pytest.mark.parametrize('chunk', [True, False]) | ||
| def test_dask_percentile_implemented_faster_xr_quantile(control3d, chunk): | ||
| chunk_dim, dim = 'x', 'time' | ||
| # chunk_dim, dim = 'time', 'x' # fails, why? |
There was a problem hiding this comment.
my_quantile works as expected in bootstrap_compute because it is applied to the first dimension (there bootstrap, here time). but it fails to work on axis!=0 with nans.
this small demo runs without asserion errors:
a.dims ('time', 'lon', 'lat')
a.shape
q = .05
chunk_dim, dim = 'lon', 'time'
ac = a.chunk({chunk_dim: 2})
acp = a.quantile(dim=dim,q=q)
acp.shape
%time acp2 = my_quantile(ac, dim, q).compute()
acp2.shape
xr.testing.assert_allclose(acp, acp2)
acl = ac.load()
%time acp = my_quantile(acl, dim, q).compute()
acp.shape
xr.testing.assert_allclose(acp, acp2)
chunk_dim, dim = 'time', 'lon'
acp = a.quantile(dim=dim,q=q)
acp.shape
ac = a.chunk({chunk_dim: 2})
%time acp2 = my_quantile(ac, dim, q).compute()
acp2.shape
xr.testing.assert_allclose(acp, acp2)
acl = ac.load()
%time acp = my_quantile(acl, dim, q).compute()
acp.shape
xr.testing.assert_allclose(acp, acp2)
climpred/bootstrap.py
Outdated
| return np.percentile(arr, axis=axis, q=q) | ||
|
|
||
|
|
||
| def my_quantile(ds, dim='bootstrap', q=0.95): |
There was a problem hiding this comment.
see the tests. this implementation works great if dim is axis=0 and there are no missing values. I dont really get why tests fail.
There was a problem hiding this comment.
I don't have time to do this right now. Hopefully you'll be able to work this out when responding to comments. I can look another time after you incorporate the comments.
|
So I've mentioned this in individual comments, but right now, this just encourages/enables However, this doesn't address at all the embarassingly parallel iteration problem. I think we need to leverage something more advanced like Andrew asked this here... it may be subtle and we just need to wrap the iterations? I'll test on a small example locally with a different function. https://groups.google.com/forum/#!searchin/xarray/andrew$20huang%7Csort:date/xarray/KBwllWmMY30/v98z03lcAgAJ But my stats were: Compute function Macbook Pro: No chunking: 24.8s Casper with 36 cores/workers: No chunking: 15.7s Bootstrapping: Macbook Pro: Chunking: 2min29s Casper: Chunking: 2min57s |
There was a problem hiding this comment.
This looks like a lot of comments but each of them should only take a few seconds. I was really nitpicky here because I think it's important for us to commit really clean code with good docstrings, etc. so we don't have to worry as much about refactoring later (and so it's easy on new contributors)
My benchmark tests are failing but I think it's because it needs to be rebased. Is |
|
nope |
|
Somehow docs fail in travis but not in rtd check |
@aaronspring , this is because RTD check just makes sure that they can build. The notebook that broke is pre-compiled so that RTD doesn't need to use the memory to compile it. Travis wipes the notebook clean and tries to compile it to see if we have any breaking code. Looks like there's some breaking code in there that needs to be fixed. I can look back over this PR later today |
|
Two things that need to be fixed for travis:
CellExecutionError in examples/subseasonal/weekly-subx-example.ipynb:
------------------
fcstds=decode_cf(fcstds,'init')
fcstds['init']=pd.to_datetime(fcstds.init.values.astype(str))
fcstds['init']=pd.to_datetime(fcstds['init'].dt.strftime('%Y%m%d 00:00'))
------------------
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
<ipython-input-8-b3735a2f029c> in <module>
1 fcstds=decode_cf(fcstds,'init')
2 fcstds['init']=pd.to_datetime(fcstds.init.values.astype(str))
----> 3 fcstds['init']=pd.to_datetime(fcstds['init'].dt.strftime('%Y%m%d 00:00'))
~/miniconda/envs/climpred-dev/lib/python3.6/site-packages/pandas/core/tools/datetimes.py in to_datetime(arg, errors, dayfirst, yearfirst, utc, format, exact, unit, infer_datetime_format, origin, cache)
736 result = convert_listlike(arg, format)
737 elif is_list_like(arg):
--> 738 cache_array = _maybe_cache(arg, format, cache, convert_listlike)
739 if not cache_array.empty:
740 result = _convert_and_box_cache(arg, cache_array)
~/miniconda/envs/climpred-dev/lib/python3.6/site-packages/pandas/core/tools/datetimes.py in _maybe_cache(arg, format, cache, convert_listlike)
145 if cache:
146 # Perform a quicker unique check
--> 147 if not should_cache(arg):
148 return cache_array
149
~/miniconda/envs/climpred-dev/lib/python3.6/site-packages/pandas/core/tools/datetimes.py in should_cache(arg, unique_share, check_count)
114 assert 0 < unique_share < 1, "unique_share must be in next bounds: (0; 1)"
115
--> 116 unique_elements = set(islice(arg, check_count))
117 if len(unique_elements) > check_count * unique_share:
118 do_caching = False
TypeError: unhashable type: 'DataArray'
TypeError: unhashable type: 'DataArray' |
|
xr.quantile is now in xr 0015 daskified. Do you understand the notebook error? Haven’t touched that... |
Great.
I just tried reproducing it locally and it ran fine, so I re-triggered the travis build to see if it was just a weird error. I'll follow up after the build is done... |
|
That didn't work. @aaronspring try replacing the cell: fcstds=decode_cf(fcstds,'init')
fcstds['init']=pd.to_datetime(fcstds.init.values.astype(str))
fcstds['init']=pd.to_datetime(fcstds['init'].dt.strftime('%Y%m%d 00:00'))with: fcstds=decode_cf(fcstds,'init')I'm not convinced those |
|
this now passes. fixed with pandas==0.25.3. but there is a bug in bootstrap_hindcast for probabilistic metrics which I only found because of |
|
Thanks @aaronspring! We can keep #317 to another PR. Probably something we will talk about during OSM/your week here. I'll review/merge later today or tomorrow. Have some deadlines to meet today.. |
|
The implementation of I would prefer to implement this sneaky new |
|
This is great @aaronspring, thanks so much. Merging now! |
Description
bootstrap_funcfor stats andbootstrap_computefor skill):xr.quantileexchanged fordask.map_blocks(np.percentile)How I got there - 2 working gists:
Still to do:
my_quantilewithdimargumentWaiting for PRs to merge first:
Closes #145
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
pytest
Checklist (while developing)
pytest, if necessary.Pre-Merge Checklist (final steps)