-
Notifications
You must be signed in to change notification settings - Fork 40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: test for Grid equality #152
Conversation
powersimdata/input/grid.py
Outdated
def __eq__(self, other): | ||
"""Used when 'self == other' is evaluated. | ||
:param object other: other object to be compared against. | ||
:return: *bool*. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:return: (bool). Missing parenthesis
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -208,7 +208,7 @@ def column_name_provider(): | |||
'name', 'interconnect_sub_id', 'lat', 'lon', 'interconnect'] | |||
col_name_bus = [ | |||
'bus_id', 'type', 'Pd', 'Qd', 'Gs', 'Bs', 'zone_id', 'Vm', 'Va', | |||
'loss_zone', 'baseKV', 'Vmax', 'Vmin', 'lam_P', 'lam_Q', 'mu_Vmax', | |||
'baseKV', 'loss_zone', 'Vmax', 'Vmin', 'lam_P', 'lam_Q', 'mu_Vmax', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mislabelled these 2 column names in the bus data frame? Good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't matter for any of our purposes, but this popped up when I was trying to compare bus dataframes between a USA TAMU model and a MATReader model.
powersimdata/input/grid.py
Outdated
:return: *bool*. | ||
""" | ||
def _univ_eq(ref, test): | ||
"""Check for {boolean, dataframe, or column data} equality.""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:param ref: and :param test: are missing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, the exception raised are not listed in the docstring. This will have the benefit to help the user understand what is tested and where.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
powersimdata/input/grid.py
Outdated
|
||
if not isinstance(other, Grid): | ||
other_type = type(other).__name__ | ||
raise NotImplementedError(f'Unable to compare Grid & {other_type}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f-string won't work on server
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
It seems like Should we create a new scenario with Storage to make sure that the data in the |
@rouille you are right, we were not adequately accounting for storage. I tried comparing scenarios 90 and 159 from the server, which are identical besides the presence of storage in 159, and it was succeeding at first because I was looking to Documenting that the addition of storage causes a
I'm not immediately picturing how we test that |
Thanks for the additional test and fix. The test is even better. I was not sure that we had two exact scenarios but for storage on the server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing these tests. It looks great.
045cd2c
to
32d8c63
Compare
Purpose
Be able to compare whether two
Grid
objects are 'equal' in pythonic syntax:x == y
. This feature will be useful for our own development (e.g. #150), to ensure that any refactoring does not change how Grid objects are instantiated. It may come in handy for some other analysis functions as well, although I don't have an example off the top of my head.What is the code doing
In
grid.py
we add the__eq__
magic method to Grid objects. There is an internal_univ_eq
function which will raise an AssertionError if the two inputs are not: a) logically equal (e.g. ints or dicts), b) DataFrames with equal indexes and values, or c) DataFrames with identical indexes and the same columns and data, but with column order transposed. This last condition is relevant for our differently instantiated Grids, which load all the right data in the Plant DataFrame but transpose some columns.Then, we loop through all elements in
fields
andtransform
, ensuring that the values in them are equal (or close enough to equal, given the data modifications we expect).In
mat_reader.py
: we correct the transposition of two column names for thebus
table we read from the MAT file.Verification that this works as expected
Time to review
Approximately one hour, maybe less.