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: add get_base_grid() to states #528

Conversation

goccert25
Copy link
Contributor

Pull Request doc

Purpose

What is the larger goal of this change?
To pay down some tech debt and hopefully make PowerSimData codebase more maintainable

What the code is doing

How is the purpose executed?
Removing many similar/copy and pasted calls to initialize the original Grid that a scenario was derived from via information from the scenario object

Testing

How did you test this change (unit/functional testing, manual testing, etc.)?
Existing unit tests

Time estimate

How long will it take for reviewers and observers to understand this code change?
20 minutes

@jenhagg
Copy link
Collaborator

jenhagg commented Aug 9, 2021

Nice, the implementation looks good to me. One other thing I would consider is updating the usages of scenario.state.get_base_grid() to be simply scenario.get_base_grid().

@goccert25 goccert25 force-pushed the goccert25/add_base_grid_helper_function branch from f665895 to 26f50e8 Compare August 10, 2021 03:16
@goccert25
Copy link
Contributor Author

Nice, the implementation looks good to me. One other thing I would consider is updating the usages of scenario.state.get_base_grid() to be simply scenario.get_base_grid().

@jon-hagg I used scenario.state.get_base_grid() because in the code there was already scenario.state.get_grid() throughout. I'm happy with switching over to scenario.get_base_grid() but if so I would ask if I could also do scenario.get_grid() to be consistent. Let me know!

@danielolsen
Copy link
Contributor

The code is clear, and it simplifies the logic within the powersimdata.design module. I support a general change from scenario.state.foo to scenario.foo, which is available since #503. Previously we simplified the user-facing example code but kept the more verbose calls within the internal code so that the tests would still pass with the mock objects, but now we should be able to simplify everywhere.

One comment is that with the variable renaming from grid_new to grid within the investment cost functions, we should be sure to remember that what's in this object named grid is not really a grid in the usual sense of the word, but a grid difference for one table within the grid (e.g. the branch table or the plant table or the dcline table), which we use a Grid object for because it's convenient to have those other Grid attributes around (i.e. the bus table, sub table, other non-table attributes, etc.) within the investment cost calculation functions. One day we might decide that we want a cleaner implementation, e.g. maybe with a new GridDelta class that could have one or more tables that are really tables of element differences, not of elements themselves, but I think that's outside of the scope of this PR.

@jenhagg
Copy link
Collaborator

jenhagg commented Aug 10, 2021

@goccert25 good call - I would also support switching to scenario.get_grid().

@goccert25 goccert25 force-pushed the goccert25/add_base_grid_helper_function branch from 26f50e8 to 741aa40 Compare August 11, 2021 04:33
@goccert25 goccert25 force-pushed the goccert25/add_base_grid_helper_function branch from 741aa40 to 4399e73 Compare August 11, 2021 04:41
@goccert25
Copy link
Contributor Author

@jon-hagg done

@danielolsen renamed the grid variable to grid_differences to be more appropriate

Feel free to approve and merge whenever, I rebased on develop as of the time of this comment

Copy link
Collaborator

@jenhagg jenhagg left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@BainanXia BainanXia left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@danielolsen danielolsen left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@jenhagg jenhagg merged commit 686aa03 into Breakthrough-Energy:develop Aug 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.

4 participants