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

First draft of new climatology config #1356

Draft
wants to merge 3 commits into
base: main-dev
Choose a base branch
from
Draft

First draft of new climatology config #1356

wants to merge 3 commits into from

Conversation

dulte
Copy link
Collaborator

@dulte dulte commented Sep 30, 2024

Change Summary

Adds new way of defining how the climatology is defined. Instead of just a bool to turn climatology off and on, a climatology config can now be passed into the colocation setup.

This is a reimplementation of @Ovewh 's PR #1135. As pyaercom has changed as much as it has, it was easier to do everything from scratch.

Related issue number

Aims to fix #1125

Checklist

  • Start with a draft-PR
  • The PR title is a good summary of the changes
  • PR is set to AeroTools and a tentative milestone
  • Documentation reflects the changes where applicable
  • Tests for the changes exist where applicable
  • Tests pass locally
  • Tests pass on CI
  • At least 1 reviewer is selected
  • Make PR ready to review

@dulte dulte self-assigned this Sep 30, 2024
@dulte
Copy link
Collaborator Author

dulte commented Sep 30, 2024

Two tests fail due to different mean obs than expected with new climatology.

@dulte dulte added bug 🐛 Something isn't working enhancement New feature or request labels Sep 30, 2024
@dulte dulte added this to the m2024-11 milestone Sep 30, 2024
@Ovewh
Copy link
Collaborator

Ovewh commented Sep 30, 2024

Two tests fail due to different mean obs than expected with new climatology.

I remember climatology previously was hard coded, in the pyaerocom const to be defined as the mean between 2005- 2015, so if you use different period it might cause the test failing.

The calc_climatology function in helpers.py will set the year of the climatology unless defined (the set_year keyword), to be the middle of the the period which means that unless the year of the model data you want to compare is also that year the collocation will fail. This also causes issues if you like extend the period for which the climatology is defined i.e. from default of 2005-2015 to being 2005 - 2020. Then the time dimension of the gridded data object also would have to update if the collocation is to succeed.


resample_how: str = const.CLIM_RESAMPLE_HOW
freq: str = const.CLIM_FREQ
mincount: dict = const.CLIM_MIN_COUNT
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think set_year should also be an attribute here, or if you have a better solution?

stop=use_climatology_ref.stop,
clim_mincount=use_climatology_ref.mincount,
resample_how=use_climatology_ref.resample_how,
clim_freq=use_climatology_ref.freq,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here set_year should be set

freq: str = const.CLIM_FREQ
mincount: dict = const.CLIM_MIN_COUNT

@field_validator("resample_how")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need a validator for this. Instead

resample_how: Literal["mean", "median"] = const.CLIM_RESAMPLE_HOW

assuming const.CLIM_RESAMPLE_HOW is either "mean" or "median"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

use_obs_clim is broken and has always been: Turns all obsdata into nan
3 participants