[PR]: Enable `skipna` for spatial and temporal mean operations

xCDAT / xcdat

An extension of xarray for climate data analysis on structured grids.

https://xcdat.readthedocs.io/en/latest/

Apache License 2.0

101 stars 11 forks source link

[PR]: Enable `skipna` for spatial and temporal mean operations #655

Open lee1043 opened 1 month ago

lee1043 commented 1 month ago

Description

Enable skipna parameter for mean operation.

Closes #582

Checklist

[x] My code follows the style guidelines of this project
[x] I have performed a self-review of my own code
[x] My changes generate no new warnings
[x] 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)
[x] I have commented my code, particularly in hard-to-understand areas
[x] 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)

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (c653bf2) to head (2013950).

:exclamation: Current head 2013950 differs from pull request most recent head 7f9d2e3

Please upload reports for the commit 7f9d2e3 to get more accurate results.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #655 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 15 15 Lines 1544 1544 ========================================= Hits 1544 1544 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

lee1043 commented 1 month ago

Test code attached: Archive.zip

tomvothecoder commented 1 month ago

Thanks for this PR @lee1043! I will more closely either some time at the end of this week or early next week.

pochedls commented 1 month ago

@lee1043 – Thanks for taking on this PR. This seems like a handy option to have in xcdat. I just took a quick look at this – it seems like skipna should be added to the docstring (since it is now available in the public API). I'm also curious if you can copy a couple existing unit tests, add a np.nan value somewhere, and ensure that the NaN value propagates to the final result (I think this is the expected behavior).

lee1043 commented 1 month ago

@pochedls thanks for the suggestion. I will work on these and ping you and @tomvothecoder for review once ready.

tomvothecoder commented 1 week ago

I'm assuming that we want to add the skipna arg to group_average(), climatology(), and departures() (in addition to average()). Is this correct @lee1043 and @pochedls? I made these updates in this commit 7f9d2e3 (#655) along with adding unit tests.

pochedls commented 1 week ago

I'm assuming that we want to add the skipna arg to group_average(), climatology(), and departures() (in addition to average()). Is this correct @lee1043 and @pochedls? I made these updates in this commit 7f9d2e3 (#655) along with adding unit tests.

I think so. This probably needs to be tested on real data (I can't think of unintended consequences, but it is worth stress testing a little).