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

Refactor bugfix/150-refactor-spatial-avg (PR #152) #154

Conversation

tomvothecoder
Copy link
Collaborator

@tomvothecoder tomvothecoder commented Nov 16, 2021

Description

This PR is a refactoring effort of PR #152.

Summary of Changes

  • Update spatial_avg() weights kwarg default val to "generate"
  • Refactor get_weights()
    • Update so that it only gets weights based on axis arg
    • Add _get_longitude_weights() and _get_latitude_weights()
    • Add _calculate_weights()
  • Update combine_weights() to not parse the axis_weights() dict
    • Only the relevant weights from get_weights() based on specified axis arg
  • Refactor _align_longitude_to_360_axis()
  • Update _swap_lon_axes to _swap_lon_axis

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

If applicable:

  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass with my changes (locally and CI/CD build)
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

@tomvothecoder tomvothecoder added type: bug Inconsistencies or issues which will cause an issue or problem for users or implementors. Priority: Medium labels Nov 16, 2021
@tomvothecoder tomvothecoder self-assigned this Nov 16, 2021
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
@tomvothecoder
Copy link
Collaborator Author

@pochedls This PR is still in progress, but you can check out the changes so far and my latest review comments.

pmb_index = None
return pmb_index, d_bounds
prime_meridian = None
return d_bounds, prime_meridian
Copy link
Collaborator Author

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.

xcdat/spatial_avg.py Outdated Show resolved Hide resolved
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
@tomvothecoder tomvothecoder marked this pull request as ready for review November 17, 2021 19:33
@tomvothecoder
Copy link
Collaborator Author

Hi @pochedls, this PR is ready for review.

xcdat/spatial_avg.py Outdated Show resolved Hide resolved
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
@pochedls
Copy link
Collaborator

@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 data_vars="minimum" (and perhaps a fix to #158 on this branch for testing).

@tomvothecoder
Copy link
Collaborator Author

@pochedls Yes, you can rebase your branch with main and I'll rebase this branch onto your's.

I'll take a closer look at #158 and see if it warrants a separate PR.

@pochedls
Copy link
Collaborator

@pochedls Yes, you can rebase your branch with main and I'll rebase this branch onto your's.

I'll take a closer look at #158 and see if it warrants a separate PR.

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.

@tomvothecoder tomvothecoder force-pushed the bugfix/150-refactor-spatial-avg-tv branch from 5b64fe6 to 91c3d5d Compare November 18, 2021 16:43
@tomvothecoder
Copy link
Collaborator Author

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.

@pochedls This branch has been rebased on your branch. If you're open to it, you can give #158 a shot!

tests/test_spatial_avg.py Outdated Show resolved Hide resolved
xcdat/spatial_avg.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@pochedls pochedls left a 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"`
@tomvothecoder tomvothecoder force-pushed the bugfix/150-refactor-spatial-avg-tv branch from 0a0c8da to 964a4f4 Compare November 22, 2021 22:27
@tomvothecoder tomvothecoder merged commit 6244c6f into bugfix/150-refactor-spatial-avg Nov 22, 2021
@tomvothecoder tomvothecoder deleted the bugfix/150-refactor-spatial-avg-tv branch November 22, 2021 22:32
pochedls added a commit that referenced this pull request Nov 23, 2021
…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]>
@tomvothecoder tomvothecoder linked an issue Nov 24, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Inconsistencies or issues which will cause an issue or problem for users or implementors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update default value of weights kwarg in spatial_avg()
2 participants