Add validation test for emissions factor references provided in zone configs #6001

jayaddison · 2023-10-11T10:51:46Z

Description

While developing #5987 it was nontrivial for me to check whether the source field defined for an emission factor matched an existing top-level sources entry in the zone.

This pull request provides a test case to perform that validation automatically, and also caters for a number of existing situations where any of the three of {averaged Electricity Maps data, or implicit well-known source references, or assumptions} are made about the emissions factor estimates.

Double check

~~I have tested my parser changes locally with poetry run test_parser "zone_key"~~
I have run ~~pnpx prettier --write . and~~ poetry run format to format my changes.

… config emissions factor entries

jayaddison · 2023-10-11T12:22:23Z

tests/test_zones_json.py

@@ -7,6 +7,24 @@


 class ZonesJsonTestcase(unittest.TestCase):
+    # Add well-known sources that don't require config-based references here
+    GLOBAL_SOURCE_REFERENCES = {


I think it'd be preferable to create a separate YAML reference sources file that contains these and any relevant metadata -- extracting a constant set here to hold them is something of a migration phase to figure out how many items exist and to spot patterns in them.

After more thought: first it'd make sense to check whether this would be a worthwhile and acceptable change.

… pass

…ce classes defined above it within the module

…ctors stored directly on zone config objects as read from YAML

tests/test_config_model.py

This reverts commit efd56dc.

This reverts commit 2de8e37.

…sions factors stored directly on zone config objects as read from YAML" This reverts commit 88ea0a6.

… reference classes defined above it within the module" This reverts commit 3ca1990.

…ation to pass" This reverts commit e4e701c.

… is loaded" This reverts commit 1464290.

…ODEL

…rence, so assert that it is a required value

…d a link to the calculation that derived it While doing this I also confirmed that HOPS is an ENTSOE transmission system operator in Croatia; the linked document is also available on their English-language homepage under Documents -> Basic Data at 'http://hops.hr/page-file/iXgT7cSSC4U28AfzW8lBZ6/publications/Temeljni_podaci_2018.pdf' Cross-references-commit d3072b4.

… URL by defining 'sources' entries for it per-zone Also confirmed while doing this that the comment is accurate, in that the percentages mentioned refer to the Y2020 energy mix reported by the charts at the given URL

…lectricity supply technologies

…atemalan AMM (Administrador del Mercado Mayorista) The link is to the page containing the API where data was extracted from and that belongs to the authority source; the docs.google.com URL for the derived analysis remains as a _url property on the relevant emissions factors

…e configuration files for Japan

…ng the sentence, hopefully with no loss of information (and retaining the explicit disclaimer note)

…ly-correct structure

… (that was a temporary measure to allow the unit tests to pass while cleaning up the zone config files)

jayaddison · 2023-10-13T10:25:56Z

config/zones/UA.yaml

@@ -90,7 +90,7 @@ emissionFactors:
      _comment: estimated yearly share of solar & wind in this mixed renewable category
        based on installed capacity in 2018
      _url: https://en.wikipedia.org/wiki/Renewable_energy_in_Ukraine
-      source: renewable mix; assumes 50% solar, 50% wind
+      source: assumes 50% solar, 50% wind renewable mix


According to figures from the latest currently-available iteration of the same Wikipedia source page, in Y2021 the ratio of solar:wind capacity was 6227:1673.

However: those figures predate the conflict in Ukraine that began in Y2022 and so the numbers may not correspond to the situation more recently. Note also that data for Ukraine is currently unavailable on Electricity Maps (the parser has been down for a while; I'm not cross-linking the relevant issue thread here because it's not relevant to most of this pull request).

Also: installed capacity ratio doesn't necessarily indicate actual output ~~capacity~~. So even though the capacity ratio may have changed, the ratio itself may not be suitable to map 1-1 to the percentages assumed here.

…x in UA

jayaddison · 2023-10-13T10:39:54Z

config/zones/IN-KA.yaml

-        - http://srldc.org/2018-19srallocation.aspx
-      source: calculated 20% nuclear, 80% thermal (coal)
+      _comment: calculated 20% nuclear, 80% thermal (coal)
+      source: Souther Regional Load Dispatch Center Allocation Limits 2018-2019; SRLDC 2018-2019 derived calculation for Karnataka


@zuron7 this is data you collected a few years ago now, but I wondered whether you know where there might be updated data for Karnataka from SRLDC (or elsewhere)? It looks like their website has moved from srldc.org to srldc.in, and I'm having difficulty accessing it.

…are required (not optional) on each CO2 parameters model

jayaddison · 2023-10-13T15:40:03Z

I'm going to open this as 'ready for review'. Initially my goal here was solely to write a test case to make it easier to detect source-related problems - along the way the work has branched off into a few other source related cleanups and questions, but I think I should pause before going any further into the details on those.

VIKTORVAV99 · 2023-10-13T15:50:30Z

Since as you mentioned this has gotten a little bigger than initially intended I won't have time to review it today but at first glance everything seems pretty straightforward. So hopefully I can test and review it this weekend.

jayaddison · 2023-10-13T15:54:44Z

Thanks @VIKTORVAV99 - it can (dare I say should?) wait until next week, though.

VIKTORVAV99

Works really well, most of the changes are straight forward and nothing special to note.

Just some small comments but nothing that should affect functionality.

VIKTORVAV99 · 2023-10-15T20:14:28Z

tests/test_config_model.py

+                                continue
+                            if source in self.GLOBAL_SOURCE_REFERENCES:
+                                continue
+                            self.assertIn(source, CONFIG_MODEL.zones[zone_key].sources)


This line here has a type error and prints the following using pylance:

Argument of type "dict[str, Source] | None" cannot be assigned to parameter "container" of type "Iterable[Any] | Container[Any]" in function "assertIn"

Ok; and it seems that this is due to the fact that the field is optional (an x in foo check is invalid if foo is None). But it is correct for it to be an optional field. And pyright isn't convinced-enough by adding a assertIsNotNone on the previous line to eliminate the | None possibility.

Thinking about how to resolve this.. (adding or {} to the end of this line would be a quick although not-ideal way).

And pyright isn't convinced-enough by adding a assertIsNotNone on the previous line to eliminate the | None possibility.

See microsoft/pyright#2007 for the (reasonable) rationale behind that. However, the workaround suggested (add an assert ... is not None - attempted locally as assert CONFIG_MODEL.zones[zone_key].sources is not None) does not seem to be working for me. I'll try to figure out whether that's a pyright bug.

Yep, I think there could be a pyright bug here. assert .. is not None doesn't seem to preserve the asserted non-empty condition in the case where attribute access is used here:

zone_sources = CONFIG_MODEL.zones[zone_key].sources assert CONFIG_MODEL.zones[zone_key].sources is not None assert zone_sources is not None self.assertIn(source, CONFIG_MODEL.zones[zone_key].sources) # pyright complains here self.assertIn(source, zone_sources) # pyright does not complain here

Hmm. Maybe that's as-expected - possibly because the pydantic.BaseModel that Zone derives from implements __getattr__, so the results of attribute access could change from call to call.

Ok, this should now be resolved (96974de - hold the container's value in a local variable and use an assert ... is not None to help pyright narrow the typechecking for it).

VIKTORVAV99 · 2023-10-15T20:16:22Z

tests/test_config_model.py

+    }
+
+    def test_zone_sources(self):
+        for measurement_basis, model in CO2EQ_CONFIG_MODEL:


Since measurement_basis is not used I would underscore it.

Ah, my mistake committing a partial change-in-progress there: I was considering adding that variable to the error message here to help people figure out where the source entry is missing. What do you think?

Hopefully resolved; I think that making the error messages from this unit test as unambiguous as possible would make sense, so I've standardized the format of them a bit, and included the full path to the source key when it's missing (making use of the measurement_basis variable).

…when reference sources are found

…th to missing 'source' key

VIKTORVAV99

LGTM! 🎉

Thanks for making the changes!

Edit: Merging as soon as the merge conflicts are resolved. :)

jayaddison · 2023-10-23T20:32:43Z

Thank you @VIKTORVAV99! Taking a look at resolving the merge conflicts now.

jayaddison · 2023-10-23T21:06:58Z

I've resolved the conflicts locally, although tests fail (as intended, I think) due to a recently-added CH zone source without a top-level definition in the zone config.

I'll wait to hear back from the related CH zone thread to confirm whether there's a preferred way to attribute that updated source.

pierresegonne · 2023-10-24T06:57:24Z

I think I understand what you had in mind for CH. Yes you can definitely create:

sources:
  - SFOE, Electricity Maps:
    link: ....

And then only reference SFOE, ELectricity Maps in the individual entries for emission factors

…rived from Electricity Maps worksheet with SFOE contribution

jayaddison · 2023-10-24T09:49:46Z

Thanks @pierresegonne - yep, you understood my question :) (I also read your earlier comment before this).

jayaddison · 2023-10-24T09:55:38Z

Note: if this test causes too much friction in future when editing zone configs, please feel free to skip/remove them, I won't be offended.

On the other hand, if they turn out to be particularly useful, I'd be willing to try refactoring the logic into a pydantic field validator (like these I see in the events module) but I think it's worth figuring out whether the test is valuable, first.

VIKTORVAV99 · 2023-10-24T17:53:24Z

Note: if this test causes too much friction in future when editing zone configs, please feel free to skip/remove them, I won't be offended.

On the other hand, if they turn out to be particularly useful, I'd be willing to try refactoring the logic into a pydantic field validator (like these I see in the events module) but I think it's worth figuring out whether the test is valuable, first.

I just merged it so let's let it run a few weeks maybe (we don't get a ton of updates to the config files). But at least in my opinion the more work that can be done by CI (and is reliable) the more time we can spend doing other things like more in depth reviews or developing new features for example.

Thanks once again!

jayaddison added 3 commits October 11, 2023 11:40

zone config: retain 'emissionFactors' key after configuration is loaded

1464290

tests: add test case to validate the reference sources listed in zone…

2a483ce

… config emissions factor entries

Run 'poetry run format'

4075bdc

github-actions bot added tests zone config Pull request or issue for zone configurations labels Oct 11, 2023

jayaddison commented Oct 11, 2023

View reviewed changes

jayaddison added 7 commits October 12, 2023 14:55

Merge branch 'master' into tests/emissions-factor-reference-validation

eb39522

Zone model: add emissionFactors field to allow pydantic validation to…

e4e701c

… pass

config.model: relocate 'Zone' class definition so that it can referen…

3ca1990

…ce classes defined above it within the module

config.model: add ZoneEmissionFactors model to represent emissions fa…

88ea0a6

…ctors stored directly on zone config objects as read from YAML

config.model: cleanup: remove outdated comment

2de8e37

config.model: cleanup: use snake-casing

efd56dc

tests: migrate to a pydantic-model-based reference-testing approach

009b652

jayaddison commented Oct 12, 2023

View reviewed changes

tests/test_config_model.py Show resolved Hide resolved

jayaddison added 16 commits October 12, 2023 22:08

Revert "config.model: cleanup: use snake-casing"

bf3d534

This reverts commit efd56dc.

Revert "config.model: cleanup: remove outdated comment"

dc894e1

This reverts commit 2de8e37.

Revert "config.model: add ZoneEmissionFactors model to represent emis…

a56fb1b

…sions factors stored directly on zone config objects as read from YAML" This reverts commit 88ea0a6.

Revert "config.model: relocate 'Zone' class definition so that it can…

9c45e87

… reference classes defined above it within the module" This reverts commit 3ca1990.

Revert "Zone model: add emissionFactors field to allow pydantic valid…

ec67a64

…ation to pass" This reverts commit e4e701c.

Revert "zone config: retain 'emissionFactors' key after configuration…

24579d3

… is loaded" This reverts commit 1464290.

tests: refactor zone reference-testing to use existing CO2EQ_CONFIG_M…

46a5e02

…ODEL

tests: apply isort-preferred module formatting

9ea16a0

tests: every zone production mode estimate must provide a source refe…

d375bff

…rence, so assert that it is a required value

config: refactor-out references to IPCC 2014 Ocean (includes tidal) e…

a40152b

…lectricity supply technologies

config: refactor-out references to Enerdata as a data source from zon…

cd94f6b

…e configuration files for Japan

tests: factor-out special-case Electricity Maps reference by rephrasi…

bb9317e

…ng the sentence, hopefully with no loss of information (and retaining the explicit disclaimer note)

config: rephrase CA-AB source assumption note into a more grammatical…

86df047

…ly-correct structure

config: fixup: data for IN-KA shouldn't be described as an assumption…

cf04019

… (that was a temporary measure to allow the unit tests to pass while cleaning up the zone config files)

jayaddison commented Oct 13, 2023

View reviewed changes

config: factor-out sources for assumed 50-50 solar:wind renewables mi…

1de0de2

…x in UA

jayaddison commented Oct 13, 2023

View reviewed changes

jayaddison mentioned this pull request Oct 13, 2023

Update IN-SO emission factors #5447

Merged

2 tasks

tests: cleanup: 'emission_factors' and the 'zone_overrides' subfield …

42e27fe

…are required (not optional) on each CO2 parameters model

jayaddison mentioned this pull request Oct 13, 2023

Update CH emission factor #4037

Merged

jayaddison marked this pull request as ready for review October 13, 2023 15:40

VIKTORVAV99 self-requested a review October 13, 2023 15:49

VIKTORVAV99 reviewed Oct 15, 2023

View reviewed changes

jayaddison changed the title ~~Draft: add validation test for emissions factor references provided in zone configs~~ Add validation test for emissions factor references provided in zone configs Oct 16, 2023

jayaddison added 3 commits October 16, 2023 12:25

tests: add assertion that top-level sources configuration is present …

96974de

…when reference sources are found

tests: lint fixup: comment brevity to fix within line length limits

423c304

tests: nitpick: standardize error message format, and include full pa…

b0acba5

…th to missing 'source' key

VIKTORVAV99 self-requested a review October 19, 2023 07:30

VIKTORVAV99 approved these changes Oct 23, 2023

View reviewed changes

Merge branch 'master' into tests/emissions-factor-reference-validation

ff5cc0c

jayaddison mentioned this pull request Oct 23, 2023

Update unknown emission factor in CH #6032

Merged

config: add top-level 'source' entry to CH zone config for results de…

e483392

…rived from Electricity Maps worksheet with SFOE contribution

VIKTORVAV99 merged commit e673724 into electricitymaps:master Oct 24, 2023
17 checks passed

jayaddison deleted the tests/emissions-factor-reference-validation branch October 24, 2023 22:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add validation test for emissions factor references provided in zone configs #6001

Add validation test for emissions factor references provided in zone configs #6001

jayaddison commented Oct 11, 2023

jayaddison Oct 11, 2023

jayaddison Oct 13, 2023

jayaddison Oct 13, 2023

jayaddison Oct 13, 2023 •

edited

Loading

jayaddison Oct 13, 2023

jayaddison commented Oct 13, 2023

VIKTORVAV99 commented Oct 13, 2023

jayaddison commented Oct 13, 2023

VIKTORVAV99 left a comment

VIKTORVAV99 Oct 15, 2023

jayaddison Oct 16, 2023

jayaddison Oct 16, 2023

jayaddison Oct 16, 2023

jayaddison Oct 16, 2023

jayaddison Oct 16, 2023

VIKTORVAV99 Oct 15, 2023

jayaddison Oct 16, 2023

jayaddison Oct 18, 2023

VIKTORVAV99 left a comment •

edited

Loading

jayaddison commented Oct 23, 2023

jayaddison commented Oct 23, 2023

pierresegonne commented Oct 24, 2023

jayaddison commented Oct 24, 2023

jayaddison commented Oct 24, 2023

VIKTORVAV99 commented Oct 24, 2023

Add validation test for emissions factor references provided in zone configs #6001

Add validation test for emissions factor references provided in zone configs #6001

Conversation

jayaddison commented Oct 11, 2023

Description

Double check

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayaddison Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayaddison commented Oct 13, 2023

VIKTORVAV99 commented Oct 13, 2023

jayaddison commented Oct 13, 2023

VIKTORVAV99 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

VIKTORVAV99 left a comment • edited Loading

Choose a reason for hiding this comment

jayaddison commented Oct 23, 2023

jayaddison commented Oct 23, 2023

pierresegonne commented Oct 24, 2023

jayaddison commented Oct 24, 2023

jayaddison commented Oct 24, 2023

VIKTORVAV99 commented Oct 24, 2023

jayaddison Oct 13, 2023 •

edited

Loading

VIKTORVAV99 left a comment •

edited

Loading