Obs only #593

dulte · 2022-03-04T14:06:18Z

Way of handling dummy data to be able obs only evaluation. See #436

…n-dev

…true of no model is found

dulte · 2022-03-04T14:09:20Z

The _make_dummy_model function is only partially annotated, since importing EvalSetup lead to a circular import

codecov · 2022-03-04T14:16:46Z

Codecov Report

Merging #593 (eaa8f4f) into main-dev (27a7dac) will decrease coverage by 0.27%.
The diff coverage is 31.63%.

@@             Coverage Diff              @@
##           main-dev     #593      +/-   ##
============================================
- Coverage     76.98%   76.70%   -0.28%     
============================================
  Files            97       97              
  Lines         17494    17589      +95     
============================================
+ Hits          13467    13492      +25     
- Misses         4027     4097      +70

Flag	Coverage Δ
unittests	`76.70% <31.63%> (-0.28%)`	⬇️

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

Impacted Files	Coverage Δ
pyaerocom/helpers.py	`72.56% <8.33%> (-3.92%)`	⬇️
pyaerocom/aeroval/helpers.py	`65.00% <35.89%> (-27.86%)`	⬇️
pyaerocom/aeroval/experiment_output.py	`87.10% <50.00%> (-0.37%)`	⬇️
pyaerocom/aeroval/experiment_processor.py	`73.43% <50.00%> (-6.96%)`	⬇️
pyaerocom/aeroval/setupclasses.py	`93.33% <100.00%> (+0.11%)`	⬆️
pyaerocom/griddeddata.py	`61.38% <100.00%> (ø)`
pyaerocom/_lowlevel_helpers.py	`82.48% <0.00%> (-0.51%)`	⬇️
pyaerocom/config.py	`83.39% <0.00%> (-0.17%)`	⬇️
pyaerocom/io/read_eea_aqerep_base.py	`56.52% <0.00%> (+0.13%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74d1283...eaa8f4f. Read the comment docs.

avaldebe

I have some minor comments about the code itself.
More importantly, I would appreciate the private functions to be collected on a private module with a descriptive name.

pyaerocom/aeroval/experiment_output.py

avaldebe · 2022-03-04T14:32:24Z

pyaerocom/aeroval/experiment_processor.py

        obs_list = self.cfg.obs_cfg.keylist(obs_name)
+        if self.cfg.model_cfg == {}:


you could use the fact that an empty dictionary "is falsy"

if not self.cfg.model_cfg: ...

this also covers the case when self.cfg.model_cfg is None

Is it possible for the model_cfg attribute to be missing?

I don't think it is possible. I think aeroval will crash if no model_cfg is given

and model_cfg = None?

pyaerocom/aeroval/helpers.py

pyaerocom/griddeddata.py

avaldebe · 2022-03-04T14:51:19Z

pyaerocom/helpers.py

+    start = min([int(per.split("-")[0]) for per in periods])
+    stop = max(
+        [int(per.split("-")[1]) if len(per.split("-")) > 1 else int(per) for per in periods]
+    )


you do not need to pass a list to min/max a generator expression will work just as well

start = min(int(per.split("-")[0]) for per in periods) stop = max( int(per.split("-")[1]) if "-" in per 1 else int(per) for per in periods )

pyaerocom/helpers.py

dulte · 2022-03-07T11:40:46Z

I think I've made all the changes requested

avaldebe

Minor requests regarding the use of pathlib.Path instead of os.path.*.

Please, feel free to ignore them if you do not see the added value

pyaerocom/aeroval/helpers.py

avaldebe · 2022-03-07T12:15:48Z

pyaerocom/aeroval/helpers.py

+    outdir = os.path.join(tmpdir, f"{model_id}/renamed")
+
+    os.makedirs(outdir, exist_ok=True)


not sure if GriddedData.to_netcdf accepts pathlib.Path objects but it might be cleaner to write this part with a Path object

outdir = Path(tmpdir) / f"{model_id}/renamed" outdir.mkdir(parents=True, exist_ok=True)

GriddedData.to_netcdf uses os.path.join, so I think it must be str. If we are going to change this, then it should be its own PR

os.path.join accepts path-like objects (see docs),
which means that it will accept pathlib.Path objects

why not using a Path object?

outdir = Path(tmpdir) / f"{model_id}/renamed" outdir.makedir(parents=True, exist_ok=True)

as I wrote on my previous review

GriddedData.to_netcdf uses os.path.join, so I think it must be str. If we are going to change this, then it should be its own PR

os.path.join accepts path-like objects (see docs),
which means that it will accept pathlib.Path objects

avaldebe

small nitpicks

avaldebe · 2022-03-22T09:24:55Z

pyaerocom/aeroval/helpers.py

+    outdir = os.path.join(tmpdir, f"{model_id}/renamed")
+
+    os.makedirs(outdir, exist_ok=True)


why not using a Path object?

outdir = Path(tmpdir) / f"{model_id}/renamed" outdir.makedir(parents=True, exist_ok=True)

as I wrote on my previous review

GriddedData.to_netcdf uses os.path.join, so I think it must be str. If we are going to change this, then it should be its own PR

os.path.join accepts path-like objects (see docs),
which means that it will accept pathlib.Path objects

avaldebe · 2022-03-22T09:27:53Z

pyaerocom/helpers.py

+
+
+def get_max_period_range(periods):
+    start = min([int(per.split("-")[0]) for per in periods])


you can pass the generator expression directly to min without creating a list, e.g.

start = min(int(per.split("-")[0]) for per in periods)

avaldebe · 2022-03-22T09:32:19Z

pyaerocom/helpers.py

+
+def get_max_period_range(periods):
+    start = min([int(per.split("-")[0]) for per in periods])
+    stop = max(int(per.split("-")[1]) if len(per.split("-")) > 1 else int(per) for per in periods)


you can simplify the generator expression by processing the last element on the split,
if you are concerned about invalid periods you could add an assert

assert all(per.count("-") <= 1 for per in periods), f"invalid period in {periods=}" stop = max(int(per.split("-")[-1]) for per in periods)

avaldebe · 2022-03-22T09:36:22Z

pyaerocom/helpers.py

+    if freq not in TS_TYPE_TO_PANDAS_FREQ.keys():
+        raise ValueError(f"{freq} not a recognized frequency")


you can test membership on a dictionary directly without calling .keys()

if freq not in TS_TYPE_TO_PANDAS_FREQ: raise ValueError(f"{freq} not a recognized frequency")

dulte added 8 commits September 16, 2021 14:21

First MVP of the obs-only using constant dummy model

5947523

moves creation of dummy model to aeroval helpers, and merges with mai…

a739cee

…n-dev

Adds options for removing pages on the web

0943701

Fixes gridded data crop

17c30d5

Fixes dates of dummy cube

0a9fe21

Adds option obs_only. Can either be set in config, or file be set to …

7d383c3

…true of no model is found

Some annotation

d76eb30

Removes one annotation due to circular import error

680ceb1

dulte requested a review from avaldebe March 4, 2022 14:09

avaldebe requested changes Mar 4, 2022

View reviewed changes

Small changes to clean code

67203fe

Fix dict comprihension

2e34614

avaldebe requested changes Mar 7, 2022

View reviewed changes

Makes delete_dummy_model use pathlib

eaa8f4f

avaldebe requested changes Mar 22, 2022

View reviewed changes

dulte merged commit ee732d8 into main-dev Apr 25, 2022

dulte deleted the obs-only branch April 25, 2022 07:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Obs only #593

Obs only #593

dulte commented Mar 4, 2022

dulte commented Mar 4, 2022

codecov bot commented Mar 4, 2022 •

edited

Loading

avaldebe left a comment

avaldebe Mar 4, 2022

dulte Mar 7, 2022

avaldebe Mar 7, 2022

avaldebe Mar 4, 2022

dulte commented Mar 7, 2022

avaldebe left a comment

avaldebe Mar 7, 2022

dulte Mar 8, 2022

avaldebe Mar 8, 2022

avaldebe Mar 22, 2022 •

edited

Loading

avaldebe left a comment

avaldebe Mar 22, 2022 •

edited

Loading

avaldebe Mar 22, 2022

avaldebe Mar 22, 2022

avaldebe Mar 22, 2022 •

edited

Loading

		obs_list = self.cfg.obs_cfg.keylist(obs_name)
		if self.cfg.model_cfg == {}:

		outdir = os.path.join(tmpdir, f"{model_id}/renamed")

		os.makedirs(outdir, exist_ok=True)



		def get_max_period_range(periods):
		start = min([int(per.split("-")[0]) for per in periods])

		if freq not in TS_TYPE_TO_PANDAS_FREQ.keys():
		raise ValueError(f"{freq} not a recognized frequency")

Obs only #593

Obs only #593

Conversation

dulte commented Mar 4, 2022

dulte commented Mar 4, 2022

codecov bot commented Mar 4, 2022 • edited Loading

Codecov Report

avaldebe left a 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

dulte commented Mar 7, 2022

avaldebe left a 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

avaldebe Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

avaldebe left a comment

Choose a reason for hiding this comment

avaldebe Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avaldebe Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Mar 4, 2022 •

edited

Loading

avaldebe Mar 22, 2022 •

edited

Loading

avaldebe Mar 22, 2022 •

edited

Loading

avaldebe Mar 22, 2022 •

edited

Loading