Use map_freqs in ModelMapsEngine

[lewisblake]

Change Summary

Pass the correct maps_freq in ModelMapsEngine.

Related issue number

• Short-term version of Better implementation of maps_freq: convert ts_types to TsType in EvalSetup and modify usage in code #1267
• Fix Incorrect map frequencies: ModelMapsSetup's maps_freq is not being picked up in ModelMapsEngine::_process_map_var #1259

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

[codecov]

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 78.96%. Comparing base (816b2cc) to head (1bf6fc2).
Report is 506 commits behind head on main-dev.

Files with missing lines	Patch %	Lines
pyaerocom/aeroval/modelmaps_engine.py	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           main-dev    #1268      +/-   ##
============================================
- Coverage     79.34%   78.96%   -0.39%     
============================================
  Files           133      133              
  Lines         20421    20599     +178     
============================================
+ Hits          16204    16265      +61     
- Misses         4217     4334     +117     

Flag	Coverage Δ
unittests	78.96% <87.50%> (-0.39%)	⬇️

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]

Looks like it is being passed down to the outputted config correctly as well:

  "modelmaps_opts": {
        "maps_freq": "monthly",
        "maps_res_deg": 5
    },

[heikoklein]

The fix looks good, but I am worried that this will break many configs where maps_freq wasn't given explicitly, e.g. https://aeroval.met.no/pages/infos/?project=cams2-83&experiment=forecast-current-season has daily maps.

I suggest that maps_freq has a default of the currently used min(time_cfg.freqs, main_freq) rather than "monthly".

[lewisblake]

The CAMS2_83 case brings up a good point, the Literal["monthly", "yearly"] type-hinting won't work if we need daily maps. The "monthly" default comes from the original default in this class, so it has supposedly always been the default, even if not implemented correctly. IIRC, this default comes from input from Augustin based on a concern that Aeroval in general becomes too slow showing contour maps for longer time series. I worry that if we set a default which is too fine, and people forget about this input when setting up their config, the compute time will significantly increase, and this will only become noticeable towards the end of the processing. I'll talk with Charlie a today about what his intention was when he last modified this piece of code.

[heikoklein]

Can't we get back to the de-facto default - rather than the never implemented literal default, e.g. use the coarsest resolution, e.g.
Literal["hourly", "daily", "monthly", "yearly", "coarsest"], default "coarsest", and if coarsest is selected, we keep the old code, i.e. min(ts_type)?

[heikoklein]

Do we have some public documentation of modelmap_opts->maps_freq, e.g. in https://pyaerocom.readthedocs.io/en/latest/aeroval-examples.html (i only see it in the API/developer documentation, but not in the user-documentation for config-files).

[heikoklein]

Looks good. I would love some docs for the config. (Maybe an issue of it's to create a fully documented config page?)

[lewisblake]

No I don't see any documentation outside of the API documentation you mention. I agree, would love a full documented config. Config files are already a topic for the AeroTools workshop, and once more clearly defined, should be better documented.