extract select_ar_order #176
Conversation
Codecov Report
@@ Coverage Diff @@
## master #176 +/- ##
==========================================
+ Coverage 79.26% 79.32% +0.05%
==========================================
Files 30 30
Lines 1389 1393 +4
==========================================
+ Hits 1101 1105 +4
Misses 288 288
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| Only full models can be selected. | ||
| """ | ||
|
|
||
| from statsmodels.tsa.ar_model import ar_select_order |
There was a problem hiding this comment.
Wouldn't it be better to have the import of "ar_select_order" just after the import of xarray? To have it once and for all?
There was a problem hiding this comment.
You are right it is more common to have the imports at the beginning of the file. However, I will probably keep it here to avoid confusion because the name would be similar to select_ar_order. Tangentially relevant: #177
Just as a side remark - imports are cached in python so there is no time penalty to have it in a function as compared to the top.
|
For information, the AR order for the global variability part can be changed, but an AR order > 1 will cause issues for the spatially correlated innovations. Equation (8) of https://doi.org/10.5194/esd-11-139-2020 would have to account for highest order, and I am not sure how one could do it. |
| Only full models can be selected. | ||
| """ | ||
|
|
||
| from statsmodels.tsa.ar_model import ar_select_order |
There was a problem hiding this comment.
You are right it is more common to have the imports at the beginning of the file. However, I will probably keep it here to avoid confusion because the name would be similar to select_ar_order. Tangentially relevant: #177
Just as a side remark - imports are cached in python so there is no time penalty to have it in a function as compared to the top.
mesmer/core/auto_regression.py
Outdated
| Parameters | ||
| ---------- | ||
| data : array_like | ||
| A 1-d endogenous response variable. The independent variable. |
mesmer/core/auto_regression.py
Outdated
| Parameters | ||
| ---------- | ||
| data : array_like | ||
| A 1-d endogenous response variable. The independent variable. |
There was a problem hiding this comment.
Some last nits from my side, otherwise this is ready for review. @znicholls?
| # remove zeros | ||
| selected_ar_order.data[selected_ar_order.data == 0] = np.NaN | ||
|
|
There was a problem hiding this comment.
| # remove zeros | |
| selected_ar_order.data[selected_ar_order.data == 0] = np.NaN |
I think that does not actually happen.
|
Thanks for the reviews! |
isort . && black . && flake8CHANGELOG.rstThis PR extracts
ar_select_orderto its own numpy and xarray wrappers. There are still some open TODOs but I like the pattern that emerges in train_gv.py.