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 11 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 requested review from pochedls and tomvothecoder May 31, 2024 08:11
@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 (1cfd369) to head (b770b91).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #655   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines         1555      1556    +1     
=========================================
+ Hits          1555      1556    +1     

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

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

@pochedls
Copy link
Collaborator

Getting back to this. I don't have an appropriate validation script for this, but may be able to help with this if there is an obvious minimum validation example. For example, is there an equivalent set of CDAT commands that xcdat should be able to match here? I think we would need some reference code to develop a larger model validation script.

@tomvothecoder
Copy link
Collaborator

tomvothecoder commented Nov 6, 2024

11/6/24 Testing Requirements

Test skipna which skips missing values (as marked by NaN). It should only skip missing values for float dtypes.

The average should be calculated using non-NaN values or just return NaN for the average when there are all NaN values in a group being averaged over.

Prerequisites

  • Find equivalent set of CDAT commands to test against skipna feature
  • Develop testing with PCMDI Metrics (observation data)

APIs to test

  • spatial.spatial_avg()
  • temporal.average()
  • temporal.group_average()
  • temporal.climatology()
  • temporal.departures()

Results

TBD

@tomvothecoder
Copy link
Collaborator

tomvothecoder commented Nov 6, 2024

Just rebased this branch on the latest main. Please update your local branch accordingly.

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