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

feat: add base_grid input to investment cost calculation functions #506

Merged
merged 2 commits into from
Jun 24, 2021

Conversation

danielolsen
Copy link
Contributor

Pull Request doc

Purpose

Currently, the user-facing functions to calculate investment cost for each of {ac branches, dc branches, generation/storage} automatically compare against the base Grid in the repository. This PR adds a new parameter base_grid to each of these functions, so that another Grid can be passed instead.

This is useful because the base Grid in the repository does not correspond to any particular point in time, so calculating investment costs between for example a 2021 scenario and a 2030 scenario currently involves calculating the investment costs of each of them compared to the base grid in the repository, and then comparing those differences against each other. If instead the user can say that the 2021 scenario is the 'base' grid to compare against, then all of the implementation details of comparing the two deltas is avoided.

What the code is doing

If a base_grid is provided to any of the functions, this object is used to calculate investment costs. If this parameter is not specified, then the code proceeds as currently by spinning up a new base Grid.

Testing

No tests were performed. Currently, this instantiation of a base Grid limits the testability of the user-facing functions, since we can't pass a MockScenario with a MockGrid. This refactor improves this testability by enabling tests which avoid instantiating a Grid.

Time estimate

10 minutes.

@danielolsen danielolsen added the new feature Feature that is currently in progress. label Jun 22, 2021
@danielolsen danielolsen self-assigned this Jun 22, 2021
if base_grid is None:
base_grid = Grid(
scenario.info["interconnect"].split("_"), source=scenario.info["grid_model"]
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if base_grid is not None, do we need to make sure that the grid model is the same than the one in the scenario object?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rouille Not sure what you mean by "same". The base grid won't be the same as the one in the scenario object, unless the scenario is run with an empty change table, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the purpose of having user defined base_grid here is to provide the capability of comparing the difference between the grid in the scenario and any grid instead of the base_grid in PowerSimdata only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@BainanXia, my question is can we have a usa_tamu model in the scenario object, i.e.,scenario.get_grid().grid_model == "usa_tamu"), and a "hifld" model for the grid that is passed, i.e., base_grid.grid_model == "hifld"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, at the very least we want a check that the grid models are the same. We may also want a check that the interconnections match too, or we need to be sure that we have well-defined behavior for comparing just the overlapping parts of two Grids (e.g. comparing a Western expanded scenario to a USA base scenario).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. We now check for the matching grid model, and we should be able to handle interconnections via the reindexing logic that we add in #507.

if base_grid is None:
base_grid = Grid(
scenario.info["interconnect"].split("_"), source=scenario.info["grid_model"]
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same question here

if base_grid is None:
base_grid = Grid(
scenario.info["interconnect"].split("_"), source=scenario.info["grid_model"]
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same question here.

@rouille
Copy link
Collaborator

rouille commented Jun 24, 2021

I guess that once we merge #507 we will be able to quickly check that the scenario object that is passed to the functions in the investment_cost module is in the analyze state.

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.

Nice

@danielolsen
Copy link
Contributor Author

I guess that once we merge #507 we will be able to quickly check that the scenario object that is passed to the functions in the investment_cost module is in the analyze state.

I don't think we want to prohibit a Create state scenario from investment cost analysis, do we? All we need is get_grid().

@danielolsen danielolsen force-pushed the daniel/investment_cost_base_grid branch from 2dafacc to 1ac171f Compare June 24, 2021 17:03
@danielolsen danielolsen merged commit 88e9b7b into develop Jun 24, 2021
@danielolsen danielolsen deleted the daniel/investment_cost_base_grid branch June 24, 2021 17:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature Feature that is currently in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants