Pydantic ObsEntry #1360

lewisblake · 2024-10-02T14:39:54Z

Change Summary

Deeper work motivated by desire to generalize model naming convention.
First step of making ObsCollection and ModelCollection child classes of BaseModel
Partly related to improving the ColocatedData interface.
Rename fileconventions -> file_conventions
EvalSetup.get_obs_entry() now returns an instance of ObsEntry, not a dictionary. Clean up parts of the code which relied on this dictionary to instead, f. eks., access instance attributes.
Remove HasColocator._get_diurnal_only() and associated tests. Instead give ObsEntry an attribute diurnal_only with a default of False.

Related issue number

Part of #1085

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 · 2024-10-04T14:56:59Z

Codecov Report

Attention: Patch coverage is 85.71429% with 13 lines in your changes missing coverage. Please review.

Project coverage is 78.85%. Comparing base (af3a797) to head (114ecdd).
Report is 37 commits behind head on main-dev.

Files with missing lines	Patch %	Lines
pyaerocom/aeroval/obsentry.py	80.35%	11 Missing ⚠️
pyaerocom/aeroval/experiment_processor.py	66.66%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           main-dev    #1360      +/-   ##
============================================
+ Coverage     78.84%   78.85%   +0.01%     
============================================
  Files           136      136              
  Lines         20790    20849      +59     
============================================
+ Hits          16392    16441      +49     
- Misses         4398     4408      +10

Flag	Coverage Δ
unittests	`78.85% <85.71%> (+0.01%)`	⬆️

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.

dulte

Looks great. Some dead code. But other than that, I think it can be merged

pyaerocom/aeroval/collections.py

tests/aeroval/test_setup_classes.py

pyaerocom/aeroval/obsentry.py

thorbjoernl

Mostly nitpicking a bit, otherwise lgtm

pyaerocom/aeroval/collections.py

pyaerocom/aeroval/setup_classes.py

thorbjoernl · 2024-10-07T11:51:33Z

pyaerocom/colocation/colocation_setup.py

@@ -320,7 +320,7 @@ class ColocationSetup(BaseModel):
    @classmethod
    def validate_obs_vars(cls, v):
        if isinstance(v, str):


Maybe I just don't understand what these pydantic validators are supposed to do but shouldn't this raise an exception for unexpected input? Right now it just returns the value?

The example in field_validator docs raises ValueError for unexpected input.

obs_vars can be either a string or tuple of strings. What this is doing is coercing a provided string into a tuple of strings (for consistency). If it is not a string or a tuple of strings, then pydantic raises an error immediately.

I see, so the typehint on obs_vars is what is accepted as input in a sense? Is there a way to make vscode use the narrower set of hints that the variable actually contains after coercion? Because right now a lot of the tooling struggles with assuming that the types can be any of the original types.

I see, so the typehint on obs_vars is what is accepted as input in a sense?

Yes

Is there a way to make vscode use the narrower set of hints that the variable actually contains after coercion? Because right now a lot of the tooling struggles with assuming that the types can be any of the original types.

I'm not sure.

pyaerocom/aeroval/obsentry.py

pyaerocom/aeroval/collections.py

heikoklein

OK-ish.

This PR breaks the pyaerocom interface at several places. It does not try to create a interoperability with the old interface. What it tries, i.e. introduction of immutable code, could most likely be achieved without all of these breaks, i.e. by introducing __getitem__.

Since the broken parts are mostly internal, I approve.

Some of the comments don't make any sense to me, so maybe just remove them. Easier to understand code than comments.

heikoklein · 2024-10-07T12:11:42Z

pyaerocom/aeroval/_processing_base.py

@@ -127,7 +104,7 @@ def get_colocator(self, model_name: str = None, obs_name: str = None) -> Colocat
            mod_cfg = self.cfg.get_model_entry(model_name)
            col_cfg["model_cfg"] = mod_cfg

-            # LB: Hack and at what lowlevel_helpers's import_from was doing
+            # Hack and at what lowlevel_helpers's import_from was doing


I don't understand this comment

This has been in the code for many months now, just removing my initials. It's saying that the following lines of code were taken from a function which was broken apart.

heikoklein · 2024-10-07T12:12:14Z

pyaerocom/aeroval/_processing_base.py

-            # LB: Hack and at what lowlevel_helpers's import_from was doing
-            for key, val in obs_cfg.items():
+            # Hack and at what lowlevel_helpers's import_from was doing
+            for key, val in obs_cfg.model_dump().items():


unclear/unnecessary comment?

Same as above.

pyaerocom/aeroval/collections.py

heikoklein · 2024-10-07T12:21:51Z

pyaerocom/aeroval/obsentry.py

+
+
+class ObsEntry(BaseModel):
+    """Observation configuration for evaluation (BaseModel)


Generally, I don't like it if the interface changes (here access to all attributes from ["XXX"] to .XXX) This might destroy existing code. Ok since obsentry is mostly internal.

Agree that where the user interacts with the code, we should try to minimize the changes to the interface. But also I think it's hard to provide structure to the code if everything is fundamentally a BrowseDict without everything becoming very nested. There's a balance there, and I think this is a good place to move away from the older dictionary-style accessing.

heikoklein · 2024-10-07T12:31:04Z

pyaerocom/aeroval/obsentry.py

-        list
-            DESCRIPTION.
-
+        tuple[str, ...]
        """
        return self.obs_vars


unnecessary change of signature

return list(self.obs_vars)

would have worked as well, and didn't require the change to (v,) tuple code. A tuple[...] is in most cases better a list from a user-point perspective.

This is part of an effort to make objects in the code immutable which should not change at run time, like has been changed elsewhere.

The intention is to make obsentry immutable, which my example would also satisfy.

I agree making ObsEntry immutable is great. Now implemented.

Sadly, not as easy as I thought, but worth coming back to. #1367

thorbjoernl

LGTM 🐐

You aren't at work today.

lewisblake added 10 commits October 2, 2024 13:51

starting

dd6cac6

Start ObsEntry as BaseModel

777dedf

working through aeroval tests

e7be56a

super obs fishy WIP

832805c

simplify get_diurnal

1d6f27e

obs_id can be a tuple in case of superobs

e3ba39d

can be tuple

8c83370

WIP

51cf8b5

passes MOS tests

20176c4

rm old ObsEntry class

bdd9435

lewisblake changed the title ~~Deeper work motivated by desire to generalize model naming convention~~ Pydantic ObsEntry Oct 4, 2024

lewisblake requested a review from dulte October 4, 2024 14:52

lewisblake added this to the m2024-10 milestone Oct 4, 2024

lewisblake added the code-health 🏥 label Oct 4, 2024

lewisblake assigned lewisblake and dulte Oct 4, 2024

lewisblake requested a review from heikoklein October 4, 2024 14:55

dulte previously requested changes Oct 4, 2024

View reviewed changes

pyaerocom/aeroval/collections.py Outdated Show resolved Hide resolved

tests/aeroval/test_setup_classes.py Outdated Show resolved Hide resolved

dulte reviewed Oct 4, 2024

View reviewed changes

pyaerocom/aeroval/obsentry.py Show resolved Hide resolved

lewisblake added 6 commits October 4, 2024 17:42

clean up and validate assignment

fc2bf8d

dead code

841050d

make obs_id required

76941d7

implement obs_ver_type validator

aee0fdb

get_obs_entry returns ObsEntry instance, not a dict

88a7c45

rm _get_diurnal_only and associated tests

519df9f

lewisblake requested a review from dulte October 6, 2024 17:41

clean up doc strings

ad02462

lewisblake requested review from thorbjoernl and removed request for heikoklein October 7, 2024 10:42

thorbjoernl requested changes Oct 7, 2024

View reviewed changes

web_interface_names

895e833

heikoklein approved these changes Oct 7, 2024

View reviewed changes

feedback

aff6d5e

lewisblake requested a review from thorbjoernl October 7, 2024 13:46

lewisblake added 2 commits October 7, 2024 15:56

dead code

6256ff5

make ObsEntry immutable

114ecdd

thorbjoernl approved these changes Oct 7, 2024

View reviewed changes

lewisblake removed the request for review from dulte October 7, 2024 14:24

lewisblake merged commit bfef4c4 into main-dev Oct 7, 2024
8 checks passed

lewisblake deleted the generalize-model_naming_convention branch October 7, 2024 14:26

lewisblake restored the generalize-model_naming_convention branch October 7, 2024 14:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pydantic ObsEntry #1360

Pydantic ObsEntry #1360

lewisblake commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading

dulte left a comment

thorbjoernl left a comment

thorbjoernl Oct 7, 2024

lewisblake Oct 7, 2024

thorbjoernl Oct 7, 2024

lewisblake Oct 7, 2024 •

edited

Loading

heikoklein left a comment

heikoklein Oct 7, 2024

lewisblake Oct 7, 2024

heikoklein Oct 7, 2024

lewisblake Oct 7, 2024

heikoklein Oct 7, 2024

lewisblake Oct 7, 2024

heikoklein Oct 7, 2024

lewisblake Oct 7, 2024

heikoklein Oct 7, 2024

lewisblake Oct 7, 2024

lewisblake Oct 8, 2024

thorbjoernl left a comment



		class ObsEntry(BaseModel):
		"""Observation configuration for evaluation (BaseModel)

Pydantic ObsEntry #1360

Pydantic ObsEntry #1360

Conversation

lewisblake commented Oct 2, 2024 • edited Loading

Change Summary

Related issue number

Checklist

codecov bot commented Oct 4, 2024 • edited Loading

Codecov Report

dulte left a comment

Choose a reason for hiding this comment

thorbjoernl 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

lewisblake Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

heikoklein 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

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

Choose a reason for hiding this comment

thorbjoernl left a comment

Choose a reason for hiding this comment

lewisblake commented Oct 2, 2024 •

edited

Loading

codecov bot commented Oct 4, 2024 •

edited

Loading

lewisblake Oct 7, 2024 •

edited

Loading