Skip to content
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

refactor: remove grid fields dependency from develop #182

Merged
merged 7 commits into from
May 20, 2020
Merged

Conversation

dmuldrew
Copy link
Collaborator

@dmuldrew dmuldrew commented May 15, 2020

Purpose

This removes the recent grid field object dependencies from develop.

What is the code doing

From looking at git blame, the new changes I kept are grid equality, engine selection, removing the Seaborn dependency, and plotting color changes. It was pretty straightforward to search for grid[ to find the code using the dictionary access to grid.

Grid equality used the new objects pretty extensively so I refactored according to this current grid. I used the pandas provided assert_frame_equal test for dataframes.

Validation

All existing tests pass

Time to review

1 hour?

@dmuldrew
Copy link
Collaborator Author

I think I've migrated over the grid equality function now, though Daniel will know for sure.

@danielolsen
Copy link
Contributor

We can't use all of the same tests as in #152, because the base grid no longer matches what's in scenario 419, but the most simple test fails:

>>> from powersimdata.input.grid import Grid
>>> grid1 = Grid(['Western'])
Reading bus.csv
Reading plant.csv
Reading gencost.csv
Reading branch.csv
Reading dcline.csv
Reading sub.csv
Reading bus2sub.csv
Reading zone.csv
>>> grid2 = Grid(['Western'])
Reading bus.csv
Reading plant.csv
Reading gencost.csv
Reading branch.csv
Reading dcline.csv
Reading sub.csv
Reading bus2sub.csv
Reading zone.csv
>>> grid1 == grid2
False

@danielolsen
Copy link
Contributor

Tests were not added in #152, and that is my fault, but we want to be sure that this change does not break Grid equality. You can run simple tests that do not involve accessing scenarios on the server, and they should be able to show that two fresh copies of the grid evaluate equal, and that any modification to any of the dataframes causes two grids to evaluate as not equal.

@danielolsen
Copy link
Contributor

You can also repeat all the checks that were done in #152 by manually copying the powersimdata/input/data/usa_tamu folder contents as they were at the time. Then a fresh Grid should evaluate as equal to the grid from the server.

@danielolsen
Copy link
Contributor

I added a series of tests to the Grid equality check, see #183.

@dmuldrew
Copy link
Collaborator Author

All new grid equality tests pass now

Copy link
Collaborator

@rouille rouille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests pass, including the most recent ones testing equality between two Grid objects. I believe all modules using the fields attribute have been refacored. Thank you very much. Also, do not forget to rebase before merging.

@dmuldrew dmuldrew merged commit 08dd94b into develop May 20, 2020
@dmuldrew dmuldrew deleted the grid_redux branch May 20, 2020 19:12
@ahurli ahurli mentioned this pull request Mar 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants