-
Notifications
You must be signed in to change notification settings - Fork 12
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
Refactor bugfix/150-refactor-spatial-avg (PR #152) #154
Refactor bugfix/150-refactor-spatial-avg (PR #152) #154
Conversation
@pochedls This PR is still in progress, but you can check out the changes so far and my latest review comments. |
xcdat/spatial_avg.py
Outdated
pmb_index = None | ||
return pmb_index, d_bounds | ||
prime_meridian = None | ||
return d_bounds, prime_meridian |
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.
The return values are swapped since this method operates on the domain_bounds
if a prime_meridian
exists.
Hi @pochedls, this PR is ready for review. |
@tomvothecoder - I'm wondering if I can rebase my branch (and then this branch could be re-based). It would be helpful to have the |
I believe I correctly rebased last night - so hopefully this branch can be updated. It's okay if #158 is a separate PR - assign it to me if you'd like me to take an initial stab at it. |
5b64fe6
to
91c3d5d
Compare
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.
Thanks @tomvothecoder - this made sense and I only had a couple minor comments.
- New methods include `_get_longitude_weights()`, `_get_latitude_weights()`, and `_calculate_weights()` - Add `.copy()` for bounds dict Refactor `_align_longitude_to_360_axis()` - Update variable names for prime meridian index Update `_get_weights()` to get weights for `axis` arg Fix incorrect 'axes' refs to 'axis' Update default val of `weights` to `"generate"`
0a0c8da
to
964a4f4
Compare
…or-spatial-avg-tv
…nning prime meridian (#152) * Initial attempt at #150 * update formatting / lintering / style * add test to address missing coverage * Refactor `_get_weights()` into multiple methods - New methods include `_get_longitude_weights()` and `_get_latitude_weights()` * Update docstring * Add `.copy()` in dict * Refactor `_align_longitude_to_360_axis()` - Update variable names for prime meridian index * Add tests for latest methods - Methods include `_get_longitude_weights()`, `_get_latitude_weights()`, and `_calculate_weights()` * Fix comments and docstrings * Update `_get_weights()` to get weights for `axis` arg - Fix incorrect axes refs to axis * Update default val of `weights` to `"generate"` * Update docstring for `weights` kwarg * Refactor `_get_weights()` into multiple methods (#154) - New methods include `_get_longitude_weights()`, `_get_latitude_weights()`, and `_calculate_weights()` - Add `.copy()` for bounds dict Refactor `_align_longitude_to_360_axis()` - Update variable names for prime meridian index Update `_get_weights()` to get weights for `axis` arg Fix incorrect 'axes' refs to 'axis' Update default val of `weights` to `"generate"` * Fix pre-commit issues * Apply suggestions from code review Co-authored-by: Tom Vo <[email protected]>
Description
This PR is a refactoring effort of PR #152.
Summary of Changes
spatial_avg()
weights
kwarg default val to"generate"
weights
kwarg inspatial_avg()
#147get_weights()
axis
arg_get_longitude_weights()
and_get_latitude_weights()
_calculate_weights()
combine_weights()
to not parse theaxis_weights()
dictget_weights()
based on specifiedaxis
arg_align_longitude_to_360_axis()
_swap_lon_axes
to_swap_lon_axis
Checklist
If applicable: