First draft of new climatology config #1356

dulte · 2024-09-30T12:50:48Z

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 · 2024-09-30T12:51:23Z

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

Ovewh · 2024-09-30T14:50:02Z

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.

Ovewh · 2024-09-30T14:52:38Z

pyaerocom/climatology_config.py

+
+    resample_how: str = const.CLIM_RESAMPLE_HOW
+    freq: str = const.CLIM_FREQ
+    mincount: dict = const.CLIM_MIN_COUNT


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

Good idea. Done

Ovewh · 2024-09-30T14:54:22Z

pyaerocom/colocation/colocation_utils.py

+            stop=use_climatology_ref.stop,
+            clim_mincount=use_climatology_ref.mincount,
+            resample_how=use_climatology_ref.resample_how,
+            clim_freq=use_climatology_ref.freq,


Same here set_year should be set

pyaerocom/climatology_config.py

dulte · 2024-10-11T05:43:42Z

pyaerocom/colocation/colocation_utils.py

+    if isinstance(use_climatology_ref, ClimatologyConfig):  # pragma: no cover
+        col_freq = "monthly"  # use_climatology_ref.freq
+        obs_start = use_climatology_ref.start
+        obs_stop = use_climatology_ref.stop


This seems like an old hack or something(?). Default const.CLIM_FREQ is daily, and is thus used everywhere climatology is calculated. But the col freq is set as monthly if climatology is used. Why?

And changing col_freq in the colocate_gridded_ungridded, is that smart? Then the value from colocation config is no longer immutable(?)

Like we talked about, we should validate this asap, in the colocation_setup, so that no data is read before this change.

codecov · 2024-10-11T05:53:32Z

Codecov Report

Attention: Patch coverage is 75.60976% with 10 lines in your changes missing coverage. Please review.

Project coverage is 78.49%. Comparing base (1396c0a) to head (bbc313d).
Report is 143 commits behind head on main-dev.

Files with missing lines	Patch %	Lines
pyaerocom/climatology_config.py	72.22%	5 Missing ⚠️
pyaerocom/stationdata.py	50.00%	3 Missing ⚠️
pyaerocom/colocation/colocation_setup.py	83.33%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           main-dev    #1356      +/-   ##
============================================
- Coverage     78.97%   78.49%   -0.48%     
============================================
  Files           136      137       +1     
  Lines         20818    20891      +73     
============================================
- Hits          16441    16399      -42     
- Misses         4377     4492     +115

Flag	Coverage Δ
unittests	`78.49% <75.60%> (-0.48%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

lewisblake · 2024-10-11T07:55:39Z

pyaerocom/climatology_config.py

+        How to resample the climatology. Must be mean or median.
+    freq : str, optional
+        Which frequency the climatology should have
+    mincount : dict, optional


I'd suggest min_count. Also maybe worth building a field_validator for this? Or possibly a child class of TypedDict

lewisblake · 2024-10-11T07:57:19Z

pyaerocom/climatology_config.py

Should this module live in pyaerocom/aeroval? I don't really see this being used in the core API

As of now, it is passed though the colocator, meaning that it is part of the core. If this is moved from the core to aeroval, then some more work is needed

If that's the case then, no, it should stay here in the core. We don't want to have pyaerocom depend on pyaeroval after all.

lewisblake · 2024-10-11T07:58:38Z

pyaerocom/colocation/colocation_utils.py

+    if isinstance(use_climatology_ref, ClimatologyConfig):  # pragma: no cover
+        col_freq = "monthly"  # use_climatology_ref.freq
+        obs_start = use_climatology_ref.start
+        obs_stop = use_climatology_ref.stop


Like we talked about, we should validate this asap, in the colocation_setup, so that no data is read before this change.

First draft of new climatology config

44f0c88

dulte self-assigned this Sep 30, 2024

dulte added bug 🐛 Something isn't working enhancement ✨ New feature or request labels Sep 30, 2024

dulte added this to the m2024-11 milestone Sep 30, 2024

dulte added 2 commits September 30, 2024 14:53

Small changes to field val for climateconfig

f903e94

Removes unused deps

ef5cc9f

Ovewh reviewed Sep 30, 2024

View reviewed changes

lewisblake reviewed Oct 2, 2024

View reviewed changes

pyaerocom/climatology_config.py Outdated Show resolved Hide resolved

dulte added 3 commits October 11, 2024 06:50

Adds set_year

2b5c800

WIP

7cd2dc5

Tests run now, but colocation_utils:772 seems like an old hack

d35a7d0

dulte commented Oct 11, 2024

View reviewed changes

WIP

bbc313d

lewisblake reviewed Oct 11, 2024

View reviewed changes

lewisblake modified the milestones: m2024-11, m2024-12 Oct 11, 2024

lewisblake modified the milestones: m2024-12, m2025-01 Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First draft of new climatology config #1356

First draft of new climatology config #1356

dulte commented Sep 30, 2024

dulte commented Sep 30, 2024

Ovewh commented Sep 30, 2024 •

edited

Loading

Ovewh Sep 30, 2024

dulte Oct 11, 2024

Ovewh Sep 30, 2024

dulte Oct 11, 2024

dulte Oct 11, 2024

lewisblake Oct 11, 2024

codecov bot commented Oct 11, 2024 •

edited

Loading

lewisblake Oct 11, 2024

lewisblake Oct 11, 2024

dulte Oct 11, 2024

lewisblake Oct 20, 2024

lewisblake Oct 11, 2024

First draft of new climatology config #1356

Are you sure you want to change the base?

First draft of new climatology config #1356

Conversation

dulte commented Sep 30, 2024

Change Summary

Related issue number

Checklist

dulte commented Sep 30, 2024

Ovewh commented Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ovewh commented Sep 30, 2024 •

edited

Loading

codecov bot commented Oct 11, 2024 •

edited

Loading