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

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

lee1043
Copy link
Collaborator

@lee1043 lee1043 commented May 31, 2024

Description

Enable skipna parameter for mean operation.

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)

@lee1043 lee1043 self-assigned this May 31, 2024
@github-actions github-actions bot added the type: enhancement New enhancement request label May 31, 2024
@lee1043 lee1043 changed the title Feature/582 skipna [PR] Enable skipna for spatial and temporal mean operations May 31, 2024
@lee1043 lee1043 changed the title [PR] Enable skipna for spatial and temporal mean operations [PR]: Enable skipna for spatial and temporal mean operations May 31, 2024
Copy link

codecov bot commented May 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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
@@            Coverage Diff            @@
##              main      #655   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines         1544      1544           
=========================================
  Hits          1544      1544           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lee1043
Copy link
Collaborator Author

lee1043 commented May 31, 2024

Test code attached: Archive.zip

@tomvothecoder
Copy link
Collaborator

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

@pochedls
Copy link
Collaborator

pochedls commented Jun 3, 2024

@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
Copy link
Collaborator Author

lee1043 commented Jun 6, 2024

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

lee1043 and others added 3 commits June 5, 2024 17:34
…erator that fails CI build for python 3.9
…erator that fails CI build for python 3.9 (fixing those missed in the previous commit)
@tomvothecoder tomvothecoder changed the base branch from main to doc/600-temp-avg-info June 24, 2024 17:40
@tomvothecoder tomvothecoder changed the base branch from doc/600-temp-avg-info to main June 24, 2024 17:41
@tomvothecoder
Copy link
Collaborator

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
Copy link
Collaborator

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).

@tomvothecoder
Copy link
Collaborator

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).

Can you upload one of your model validation scripts in the xcdat-validation repo? I would like to re-use it for testing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New enhancement request
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

[Feature]: skipna parameter for averager
3 participants