Grid equality bugfix (storage) and print improvement #362

danielolsen · 2020-12-19T06:22:05Z

Purpose

Ensure that grid equality works when comparing a TAMU grid with storage and a MATReader grid with storage.
Stop printing that the grids can't be compared when really they've been found to be unequal (see https://github.com/Breakthrough-Energy/RenewableEnergyProject/issues/325#issuecomment-747871206)

What the code is doing

The ignored_subkeys are defined as a part of an AbstractGrid, and given default values in a TAMU grid. The values are used when producing storage table entries, but are not used after that. When we load a MATReader grid, we load the storage table entries, but we don't load these default values; and we shouldn't load from usa_tamu_model.py, because a MATReader is not necessarily a TAMU grid. So, when we compare Grids, we want to ignore these, since the real data to compare is in the tables, which are compared.
We remove the print, and use except Exception: rather than except: on the advice of https://www.flake8rules.com/rules/E722.html (which is the complaint that pops up on a bare except:).

Testing

To be added.

Time estimate

2 minutes.

BainanXia · 2020-12-19T18:57:39Z

powersimdata/input/grid.py

@@ -142,7 +152,6 @@ def _univ_eq(ref, test):
            _univ_eq(self.id2zone, other.id2zone)
            _univ_eq(self.bus2sub, other.bus2sub)

-        except Exception as e:
-            print(f"ERROR: could not compare grid. {str(e)}")
+        except Exception:


Good to know!

BainanXia

Looks good.

rouille · 2020-12-19T19:10:49Z

I addition to returning False, don't we want to have a print of the error so we know where is the discrepancy between the two grids that we compare?

danielolsen · 2020-12-19T19:35:24Z

I addition to returning False, don't we want to have a print of the error so we know where is the discrepancy between the two grids that we compare?

We could do something like that by wrapping each _univ_eq call in a try/except that flags each non-matching table, and then reports out at the end. As is, the current error printing is not informative, it's things like cannot compare Series with different index.

BainanXia · 2020-12-19T19:41:05Z

I addition to returning False, don't we want to have a print of the error so we know where is the discrepancy between the two grids that we compare?

We could do something like that by wrapping each _univ_eq call in a try/except that flags each non-matching table, and then reports out at the end. As is, the current error printing is not informative, it's things like cannot compare Series with different index.

In this way, we can get to know which entry can't be compared instead of returning a general error message saying cannot compare Series with different labels. I think we could either print out the first entry that gives this error then stop and leave the user to fix them one by one sequentially if an equality is expected (this is easier to implement) OR we parse through the entire grid and summarize all different entries and print out at the end (more informative).

rouille · 2020-12-19T21:33:11Z

I addition to returning False, don't we want to have a print of the error so we know where is the discrepancy between the two grids that we compare?

We could do something like that by wrapping each _univ_eq call in a try/except that flags each non-matching table, and then reports out at the end. As is, the current error printing is not informative, it's things like cannot compare Series with different index.

In this way, we can get to know which entry can't be compared instead of returning a general error message saying cannot compare Series with different labels. I think we could either print out the first entry that gives this error then stop and leave the user to fix them one by one sequentially if an equality is expected (this is easier to implement) OR we parse through the entire grid and summarize all different entries and print out at the end (more informative).

Yeah, print the first discrepancy encountered and return false would be informative.

danielolsen · 2020-12-21T18:18:32Z

Check the latest commit. Without the fixes to storage and dcline checks in #363, a comparison failure looks like:

>>> from powersimdata.scenario.scenario import Scenario
>>> old_grid = Scenario(1712).state.get_grid()
Transferring ScenarioList.csv from server
100%|#######################################| 236k/236k [00:00<00:00, 1.22Mb/s]
Transferring ExecuteList.csv from server
100%|######################################| 21.1k/21.1k [00:00<00:00, 270kb/s]
SCENARIO: test | new_bus

--> State
analyze
--> Loading grid
Loading bus
Loading plant
Loading heat_rate_curve
Loading gencost_before
Loading gencost_after
Loading branch
Loading sub
Loading bus2sub
--> Loading ct
>>> new_scenario = Scenario('')
>>> new_scenario.state.set_builder(["Texas"])
Reading bus.csv
Reading plant.csv
Reading gencost.csv
Reading branch.csv
Reading dcline.csv
Reading sub.csv
Reading bus2sub.csv
Reading zone.csv
Transferring ScenarioList.csv from server
100%|#######################################| 236k/236k [00:00<00:00, 1.56Mb/s]
--> Summary
# Existing study
test | base | Anchor | Julia
# Available profiles
demand: ercot
hydro: v1 | v2
solar: v2 | v4.1
wind: v1 | v2 | v5.1 | v5
>>> new_scenario.state.builder.change_table.add_bus([{"lat": 30, "lon": -95, "zone_id": 308}])
>>> new_scenario.state.builder.change_table.add_plant([{"type": "wind", "bus_id": 3008161, "Pmax": 400}])
>>> new_scenario.state.builder.change_table.add_branch([{"from_bus_id": 3008160, "to_bus_id": 3008161, "capacity": 300}])
>>> new_scenario.state.builder.change_table.add_storage_capacity({3008161: 100})
>>> old_grid == new_scenario.state.get_grid()
non-matching entries: dcline, storage
False

danielolsen · 2020-12-21T18:29:28Z

Plus one more commit reduces the number of try/except blocks by extending _univ_eq to add to the nonmatching_entries set directly.

rouille · 2020-12-21T19:15:52Z

powersimdata/input/grid.py

+                    assert set(ref.columns) == set(test.columns)
+                    for col in ref.columns:
+                        assert (ref[col] == test[col]).all()
+            except Exception:


It looks like Exception will always be an AssertionError, no?

If we try to compare two dataframes with transposed columns and non-identical indices, then we will get ValueError: Can only compare identically-labeled Series objects when we try to check ref[col] == test[col], before we can call .all() and check the assert. So I think we will always get either AssertionError or ValueError, in case we want to tighten up this last except.

rouille · 2020-12-21T19:25:55Z

powersimdata/input/grid.py

+        self_storage_num = len(self.storage["gencost"])
+        other_storage_num = len(other.storage["gencost"])
+        if self_storage_num == 0:
+            _univ_eq(other_storage_num, 0, "storage")


Do we need the if/else for storage. If there is no storage we still have a dict with keys that can be compared including the ones that will have empty data frames as value.

With just the changes in this PR, this doesn't work because the storage_template() in abstract_grid.py doesn't provide the same columns, but with the changes from #363 then you are right that we don't need the if/else.

If/else has been removed, as long as we merge this and #363 back-to-back there should be no problems.

Ah yeah. I reviewed #363 first and had in mind the addition you did to the storage_template function. Sorry about that.

rouille · 2020-12-21T20:10:53Z

powersimdata/input/grid.py

+                    assert set(ref.columns) == set(test.columns)
+                    for col in ref.columns:
+                        assert (ref[col] == test[col]).all()
+            except (AssertionError, ValueError):


rouille

Great. I like very much this new grid comparison. Thanks.

danielolsen added the bug Something isn't working label Dec 19, 2020

danielolsen requested review from rouille, BainanXia and jenhagg December 19, 2020 06:22

danielolsen self-assigned this Dec 19, 2020

danielolsen mentioned this pull request Dec 19, 2020

Fix MATReader when loading one Storage and/or zero DC-lines #363

Merged

danielolsen assigned jenhagg Dec 19, 2020

BainanXia reviewed Dec 19, 2020

View reviewed changes

BainanXia approved these changes Dec 19, 2020

View reviewed changes

rouille reviewed Dec 21, 2020

View reviewed changes

danielolsen force-pushed the daniel/grid_equality_fixes branch 2 times, most recently from b0db982 to f16440a Compare December 21, 2020 20:06

rouille reviewed Dec 21, 2020

View reviewed changes

rouille approved these changes Dec 21, 2020

View reviewed changes

danielolsen added 3 commits December 21, 2020 12:25

fix: remove check for unused storage subkeys in grid equality

f13b3dc

fix: add clearer printing for grid equality failure

2ca02e0

refactor: streamline branching in storage comparison

64b2ad1

danielolsen force-pushed the daniel/grid_equality_fixes branch from f16440a to 64b2ad1 Compare December 21, 2020 20:25

danielolsen merged commit e17454e into develop Dec 21, 2020

danielolsen deleted the daniel/grid_equality_fixes branch December 21, 2020 20:32

ahurli mentioned this pull request Mar 11, 2021

Develop into Master #410

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Grid equality bugfix (storage) and print improvement #362

Grid equality bugfix (storage) and print improvement #362

danielolsen commented Dec 19, 2020

BainanXia Dec 19, 2020

BainanXia left a comment

rouille commented Dec 19, 2020

danielolsen commented Dec 19, 2020

BainanXia commented Dec 19, 2020

rouille commented Dec 19, 2020

danielolsen commented Dec 21, 2020

danielolsen commented Dec 21, 2020

rouille Dec 21, 2020

danielolsen Dec 21, 2020 •

edited

Loading

danielolsen Dec 21, 2020

rouille Dec 21, 2020

danielolsen Dec 21, 2020

danielolsen Dec 21, 2020

rouille Dec 21, 2020

rouille Dec 21, 2020

rouille left a comment

Grid equality bugfix (storage) and print improvement #362

Grid equality bugfix (storage) and print improvement #362

Conversation

danielolsen commented Dec 19, 2020

Purpose

What the code is doing

Testing

Time estimate

Choose a reason for hiding this comment

BainanXia left a comment

Choose a reason for hiding this comment

rouille commented Dec 19, 2020

danielolsen commented Dec 19, 2020

BainanXia commented Dec 19, 2020

rouille commented Dec 19, 2020

danielolsen commented Dec 21, 2020

danielolsen commented Dec 21, 2020

Choose a reason for hiding this comment

danielolsen Dec 21, 2020 • edited Loading

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

rouille left a comment

Choose a reason for hiding this comment

danielolsen Dec 21, 2020 •

edited

Loading