refactor: upgrade CA-NS with event classes #6050

kruschk · 2023-10-25T02:27:01Z

Issue

Description

Upgrade the parser to use the ExchangeList, ProductionBreakdownList, ProductionMix, and ZoneKey classes. This should yield no functional change. Additionally:

Comply with Black's 88-column default line length limit.
Define global constants to replace a number of local variables and literals.
Try to use more consistent names throughout.

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.

parsers/CA_NS.py

Upgrade the parser to use the ExchangeList, ProductionBreakdownList, ProductionMix, and ZoneKey classes. This should yield no functional change. Additionally: - Comply with Black's 88-column default line length limit. - Define global constants to replace a number of local variables and literals. - Try to use more consistent names throughout. Refs: electricitymaps#6011, electricitymaps#6050

The JSON arrays a and b returned by the two endpoints appear to be parallel in the sense that element a[i] corresponds with element b[i] for all i of interest. Zip these arrays together and iterate over the resulting pairs instead of iterating over one and performing a linear search to find the corresponding element in the other. Additionally: - Check whether we've been bitten by a race condition while requesting the above array data and bail out if so. - Skip the first element of each array because the reported base load is always 0 MW. Refs: electricitymaps#6011, electricitymaps#6050

VIKTORVAV99

A few suggestions, but otherwise this looks good!

parsers/CA_NS.py

- When a pair of requests have failed, retry them until they succeed or the maximum number of attempts is exceeded. - Simplify the validation logic by making better use of the event classes instead of introducing extraneous data structures. Refs: electricitymaps#6011, electricitymaps#6050

parsers/CA_NS.py

Reliably merge the source's base load array with the corresponding electricity mix array by correlating events based on their timestamps. Refs: electricitymaps#6011, electricitymaps#6050

parsers/CA_NS.py

jarek · 2023-11-24T04:13:05Z

parsers/CA_NS.py

+        # In this source, imports are positive. In the expected result for
+        # CA-NB->CA-NS, "net" represents a flow from NB to NS, i.e., an import
+        # to NS, so the value can be used directly. Note that this API only
+        # specifies imports; when NS is exporting energy, the API returns 0.
+        exchanges.append(
+            datetime=timestamp,
+            netFlow=load * mix["Imports"] / 100,
+            source=SOURCE,
+            zoneKey=ZoneKey("CA-NB->CA-NS"),


Not strictly related to this refactor, but just to mention. The assumption that all imports are from NB is now problematic because Nova Scotia's interconnection to Newfoundland is now live. But I've never seen live data on its operation, the closest thing was that it amounted to 14% of NS supply in 2023H1, per https://www.nspower.ca/cleanandgreen/clean-energy "Our Energy Stats" chart

For reference related to your comment: the daily generation reports for Newfoundland might be usable as a source for the interconnector data, see #2541 (comment)

Hey, look: I found a page on the NSPower website that has the information you need (Maritime Link interconnector) and more, updated roughly every 5 minutes, with more granular data than the current source: it's called Daily Report, but under the heading "Current System Conditions" you'll find "10 minute averages" of e.g. load, wind, and imports and exports through various interconnectors that are at most five minutes old.

Edit: well, turns out you found it at some point as well: #3206 (comment)

So I dug deeper into this, and it turns out we aren't actually using this exchange data since we get the data on the NS-NB interconnection from New Brunswick Power, not from NSPower. So it might be best to just delete the exchanges-related part, or, alternatively, switch to obtaining exchange info from the source in #6317 (and perhaps implement the NS-Newfoundland interconnector as well). But that could be done in a separate issue.

Hey @VIKTORVAV99, any thoughts on this? Should we simply delete the exchange parser if it's not used?

To clarify, my idea was to delete the exchange parser now so we are more certain we don't accidentally fetch incorrect data, and then re-add an exchange parser based on the new source from #6317 in a separate pull request.

But doing both in this PR is also acceptable, if you've got the time for it.

Oh, gotcha! It's not entirely clear to me what needs to be done to implement the new exchange parser (there are a lot of links to follow and I'm a bit lost), so I'll just delete it here and we can address it separately. Would those changes have any impact on the production parser?

The exchange stuff shouldn't. The "Current System Conditions" also has wind generation, but as the page has data updated roughly every 5 minutes I think it'll be hard to integrate with the hourly data provided by the current source – and probably not that useful either, if the current source stays online.

As for implementing the exchange parser, the short summary is: from https://www.nspower.ca/oasis/system-reports-messages/daily-report, under "Current System Conditions", grab:

The timestamp, e.g. "Last Updated: 10-Jan-24 20:38:10";

"NS Export" for (most likely) the New Brunswick interconnector (optional, already provided by the New Brunswick parser);

"Maritime Link Import" for the Newfoundland interconnector.

The rest of the stuff in the issue is useful for exchange configuration and historical data.

Awesome, thanks for clarifying! That doesn't sound as arduous as I thought; maybe I can roll it into this PR. I'll try to carve out some time to work on it this weekend.

Alright, that wasn't so bad! A few notes:

I created the CA-NL-NF->CA-NS exchange, but this is my first time doing so, so I very well could have done something wrong. In particular, I haven't tested how it will look because I don't have the front-end set up on my machine (I used Google Maps for the coordinates and a protractor for the rotation 😁). Further, I wasn't sure whether to use CA-NL-LB or CA-NL-NF for the other zone (I chose the latter based on the physical location of the link).

I also implemented the CA-NB->CA-NS exchange, but the back-end is still configured to use the NB parser for this pair.

- Stop discarding events whose production mix percentages exceed percentage thresholds, as these imposed limits may not be sound. - Provide defaults when getting attributes from the production mix to avoid comparing numbers with Nones. Refs: electricitymaps#6011, electricitymaps#6050

- New Brunswick and Newfoundland and Labrador exchange data is available at https://www.nspower.ca/oasis/system-reports-messages/daily-report; upgrade the exchange parser to leverage this new source. - Since it no longer reduces duplication, eliminate the _get_info function and roll its logic into fetch_production. Refs: electricitymaps#2541, electricitymaps#3206, electricitymaps#6011, electricitymaps#6050, electricitymaps#6317

jarek · 2024-01-27T21:50:49Z

Can this five-month-old PR be reviewed and merged?

Current JSON endpoints aren't sending current data (#5587), if this persists, we would probably want to switch to the OASIS report being implemented in this PR, so having it merged would be a useful base.

Apply the following corrections and merge conflic resolutions: - Fix an incorrect zone in a call to fetch_exchange. - Treat CT generation as oil instead of gas. - Update the comment regarding hydro capacities. - Update the production mix thresholds for hydro and oil.

parsers/CA_NS.py

Gracefully handle errors that might occur while navigating the HTML document containing exchange data. Refs: electricitymaps#6011

parsers/CA_NS.py

- Re-throw caught errors to provide better diagnostic information. - Format log messages lazily. Refs: electricitymaps#6011

ghost · 2024-02-02T13:51:12Z

Hi, we will merge a quick fix today to make sure that the data stops being overwritten with 0s. This PR looks great and should be merged after as the quality of the refactoring is really good. :)

parsers/CA_NS.py

Refs: electricitymaps#6011

Add a check for 0 MW base loads to resolve the merge conflict. Refs: electricitymaps#6011, electricitymaps#6440

VIKTORVAV99

Looks good to me!

Thanks for bearing with me on this PR and sorry for some reviews taking longer than they should.

I'm approving this now but will let @mathilde-daugy have the final say if we should remove the in parser validation for this parser.

PS: I left some suggestions that have some tiny performance improvements as it avoid a reverse walk in the string for the value replacement.

parsers/CA_NS.py

Refs: electricitymaps#6011

kruschk · 2024-02-10T03:48:24Z

All good to merge?

github-actions bot added the parser label Oct 25, 2023

kruschk commented Oct 25, 2023

View reviewed changes

parsers/CA_NS.py Outdated Show resolved Hide resolved

kruschk added 2 commits October 24, 2023 22:15

kruschk force-pushed the kruschk/6011-ca-ns branch from a0d61d6 to 0de7079 Compare October 25, 2023 04:24

kruschk mentioned this pull request Oct 25, 2023

[Checklist]: Upgrade all parsers with event classes #6011

Open

96 tasks

VIKTORVAV99 self-requested a review October 25, 2023 07:24

VIKTORVAV99 reviewed Oct 25, 2023

View reviewed changes

parsers/CA_NS.py Outdated Show resolved Hide resolved

parsers/CA_NS.py Outdated Show resolved Hide resolved

parsers/CA_NS.py Outdated Show resolved Hide resolved

parsers/CA_NS.py Outdated Show resolved Hide resolved

VIKTORVAV99 added the hacktoberfest-accepted label Oct 30, 2023

VIKTORVAV99 self-requested a review October 31, 2023 23:01

VIKTORVAV99 reviewed Oct 31, 2023

View reviewed changes

parsers/CA_NS.py Outdated Show resolved Hide resolved

refactor: reliably correlate loads with mixes

6eb5409

Reliably merge the source's base load array with the corresponding electricity mix array by correlating events based on their timestamps. Refs: electricitymaps#6011, electricitymaps#6050

VIKTORVAV99 self-requested a review November 21, 2023 09:44

VIKTORVAV99 reviewed Nov 21, 2023

View reviewed changes

parsers/CA_NS.py Outdated Show resolved Hide resolved

jarek reviewed Nov 24, 2023

View reviewed changes

VIKTORVAV99 self-requested a review December 23, 2023 17:06

q-- mentioned this pull request Jan 6, 2024

Nova Scotia (CA-NS): Near real-time data on e.g. load, wind, and imports (not lumped together!) and exports #6317

Open

github-actions bot added python Pull requests that update Python code exchange config Pull request or issue for exchange configurations labels Jan 14, 2024

jarek mentioned this pull request Jan 27, 2024

CA-NS production parser down #5587

Open

kruschk added 2 commits January 27, 2024 16:30

Merge remote-tracking branch 'upstream/master' into kruschk/6011-ca-ns

dc2b5ce

VIKTORVAV99 reviewed Jan 28, 2024

View reviewed changes

parsers/CA_NS.py Outdated Show resolved Hide resolved

feat: handle potential HTML-navigation errors

4b60fc8

Gracefully handle errors that might occur while navigating the HTML document containing exchange data. Refs: electricitymaps#6011

VIKTORVAV99 self-requested a review January 31, 2024 07:36

Merge branch 'master' into kruschk/6011-ca-ns

c5162a7

q-- reviewed Jan 31, 2024

View reviewed changes

parsers/CA_NS.py Outdated Show resolved Hide resolved

fix: correct (and improve) error handling

792e490

- Re-throw caught errors to provide better diagnostic information. - Format log messages lazily. Refs: electricitymaps#6011

ghost reviewed Feb 2, 2024

View reviewed changes

parsers/CA_NS.py Outdated Show resolved Hide resolved

kruschk added 2 commits February 3, 2024 12:33

fix: do not fall back to a default base load

a1de3cb

Refs: electricitymaps#6011

Merge remote-tracking branch 'upstream/master' into kruschk/6011-ca-ns

96a5b6c

Add a check for 0 MW base loads to resolve the merge conflict. Refs: electricitymaps#6011, electricitymaps#6440

VIKTORVAV99 approved these changes Feb 4, 2024

View reviewed changes

parsers/CA_NS.py Outdated Show resolved Hide resolved

parsers/CA_NS.py Outdated Show resolved Hide resolved

parsers/CA_NS.py Show resolved Hide resolved

refactor: switch log messages back to f-strings

65a4769

Refs: electricitymaps#6011

Merge branch 'master' into kruschk/6011-ca-ns

8ba6905

VIKTORVAV99 enabled auto-merge (squash) February 10, 2024 08:21

VIKTORVAV99 merged commit ac7c253 into electricitymaps:master Feb 10, 2024
12 checks passed

kruschk deleted the kruschk/6011-ca-ns branch February 10, 2024 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: upgrade CA-NS with event classes #6050

refactor: upgrade CA-NS with event classes #6050

kruschk commented Oct 25, 2023

VIKTORVAV99 left a comment

jarek Nov 24, 2023

q-- Jan 5, 2024 •

edited

q-- Jan 6, 2024 •

edited

q-- Jan 6, 2024 •

edited

kruschk Jan 10, 2024

q-- Jan 10, 2024

kruschk Jan 10, 2024

q-- Jan 11, 2024

kruschk Jan 11, 2024

kruschk Jan 14, 2024

jarek commented Jan 27, 2024

ghost commented Feb 2, 2024

VIKTORVAV99 left a comment •

edited

kruschk commented Feb 10, 2024

refactor: upgrade CA-NS with event classes #6050

refactor: upgrade CA-NS with event classes #6050

Conversation

kruschk commented Oct 25, 2023

Issue

Description

Double check

VIKTORVAV99 left a comment

Choose a reason for hiding this comment

jarek Nov 24, 2023

Choose a reason for hiding this comment

q-- Jan 5, 2024 • edited

Choose a reason for hiding this comment

q-- Jan 6, 2024 • edited

Choose a reason for hiding this comment

q-- Jan 6, 2024 • edited

Choose a reason for hiding this comment

kruschk Jan 10, 2024

Choose a reason for hiding this comment

q-- Jan 10, 2024

Choose a reason for hiding this comment

kruschk Jan 10, 2024

Choose a reason for hiding this comment

q-- Jan 11, 2024

Choose a reason for hiding this comment

kruschk Jan 11, 2024

Choose a reason for hiding this comment

kruschk Jan 14, 2024

Choose a reason for hiding this comment

jarek commented Jan 27, 2024

ghost commented Feb 2, 2024

VIKTORVAV99 left a comment • edited

Choose a reason for hiding this comment

kruschk commented Feb 10, 2024

q-- Jan 5, 2024 •

edited

q-- Jan 6, 2024 •

edited

q-- Jan 6, 2024 •

edited

VIKTORVAV99 left a comment •

edited