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

Stats function moved to be better shared across metrics #1014

Merged
merged 20 commits into from
Jan 10, 2024

Conversation

lee1043
Copy link
Contributor

@lee1043 lee1043 commented Dec 23, 2023

This PR will address #1012

In addition, io functions for xcat-based xarray dataset were added via this PR. These functions can be imported by something like below:

from pcmdi_metrics.io import get_latitude, get_longitude, get_time, get_latitude_key, get_longitude_key, get_time_key, get_axis_list

@lee1043 lee1043 added this to the 3.3 milestone Dec 23, 2023
@lee1043 lee1043 self-assigned this Dec 23, 2023
@lee1043 lee1043 changed the title Stats function to be better shared across metrics Move stats function to be better shared across metrics Dec 29, 2023
@lee1043 lee1043 changed the title Move stats function to be better shared across metrics Stats function moved to be better shared across metrics Dec 29, 2023
@lee1043 lee1043 marked this pull request as ready for review January 4, 2024 19:22
@lee1043 lee1043 marked this pull request as draft January 4, 2024 19:54
@lee1043 lee1043 marked this pull request as ready for review January 4, 2024 21:36
@lee1043
Copy link
Contributor Author

lee1043 commented Jan 4, 2024

@acordonez Below functions can be imported from pcmdi_metrics.io. Could you review it that would be useful for your development?

    get_axis_list,
    get_data_list,
    get_latitude_bounds_key,
    get_latitude_key,
    get_latitude,
    get_latitude_bounds,
    get_longitude_bounds_key,
    get_longitude_key,
    get_longitude,
    get_longitude_bounds,
    get_time,
    get_time_bounds,
    get_time_bounds_key,
    get_time_key,
    select_subset,

@lee1043 lee1043 requested a review from acordonez January 4, 2024 21:38
@acordonez
Copy link
Collaborator

@acordonez Below functions can be imported from pcmdi_metrics.io. Could you review it that would be useful for your development?

    get_axis_list,
    get_data_list,
    get_latitude_bounds_key,
    get_latitude_key,
    get_latitude,
    get_latitude_bounds,
    get_longitude_bounds_key,
    get_longitude_key,
    get_longitude,
    get_longitude_bounds,
    get_time,
    get_time_bounds,
    get_time_bounds_key,
    get_time_key,
    select_subset,

@lee1043 Thanks for the links. It looks like these are getting implemented in the mean climate metrics - would it also be helpful for me to run those notebooks for a test?

@lee1043
Copy link
Contributor Author

lee1043 commented Jan 4, 2024

@acordonez statistical computation functions were relocated to under stats directory from under mean_climate directory. Sorry for mixing two different subjects in one PR. Yes, checking mean_climate notebook would be very helpful, thank you for suggesting it!

@acordonez
Copy link
Collaborator

@lee1043 This comment probably doesn't matter for regular lat/lon grids, if that's what we're targeting.

With the sea ice data, I'm still having some struggles with the get_latitude_key() and get_longitude_key() functions. For the dataset shown below, they'll return the 'i' and 'j' variables, which are not technically latitude and longitude even though they are the X and Y axis coordinates.

Screenshot 2024-01-10 at 11 38 43 AM

@acordonez
Copy link
Collaborator

@lee1043 For the rest of the functions, they are working as expected. I also ran the mean climate notebook and the results look similar to past runs.

@lee1043
Copy link
Contributor Author

lee1043 commented Jan 10, 2024

@acordonez thank you for checking this. I think I would merge this PR first and handle the sea-ice data issue in separate. Would you approve the PR if you are okay with that? If you could open an issue and point to me the example dataset (if that is different to one used previously -- I think the other one had ni and nj, so this might be a different one?) that would be greatly helpful.

@lee1043 lee1043 merged commit ea5971d into main Jan 10, 2024
5 checks passed
@lee1043 lee1043 deleted the feature/1012_lee1043_stats branch January 10, 2024 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants