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

fix: calculate investment cost for nuclear generators #391

Merged
merged 3 commits into from
Feb 9, 2021

Conversation

danielolsen
Copy link
Contributor

Purpose

Allow investment costs to be calculated for nuclear generators.

What the code is doing

The functional change is in const.py: the set gen_inv_cost_techdetails_to_keep defines the only entries to keep from the NREL ATB data, and none of the existing ones cover nuclear. The category "*" covers nuclear and nuclear only, so there should be no side effects.

The changes in test_investment_costs.py are primarily to create a test for nuclear (the changes to mock_plant and test_calculate_gen_inv_costs_2030), and secondarily to clean up the code to make it a bit more readable.

Testing

The first commit adds the test, which will fail. The second commit implements the fix and the tests pass.

Time estimate

5 minutes.

@danielolsen danielolsen self-assigned this Feb 8, 2021
10 * 679.1799258421203 * 457.1428571 * calculate_inflation(2015)
# terminals
+ 135e3 * 10 * 2 * calculate_inflation(2020)
)
dc_cost = _calculate_dc_inv_costs(mock_grid)
assert dc_cost == pytest.approx(expected_dc_cost)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are no changes here. Just reorganizing the lines, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

@@ -79,6 +79,7 @@
"LTRG1", # Single tech for wind
"4Hr Battery Storage", # Single tech for storage
"Seattle", # Single tech for solar
"*", # Single tech for nuclear
Copy link
Collaborator

Choose a reason for hiding this comment

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

That is a pretty bad choice for accessing nuclear information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear, this is not wildcard matching. The CSV file literally has an entry *.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it is terrible. I guess there is a documentation but anything else could be more explanatory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is definitely room for improvement. Currently, we're somewhat-arbitrarily selecting one TechDetail to consider for each technology, hard-coded. It would be nice to give the user more flexibility to select different ones, when they are available, or maybe even to define their own.

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.

Looks good

@danielolsen danielolsen merged commit 08b4f04 into develop Feb 9, 2021
@danielolsen danielolsen deleted the daniel/nuclear_inv_cost branch February 9, 2021 20:19
@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.

2 participants