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

Depreciation updates #16

Merged
merged 4 commits into from
Apr 12, 2024
Merged

Depreciation updates #16

merged 4 commits into from
Apr 12, 2024

Conversation

brtietz
Copy link
Collaborator

@brtietz brtietz commented Apr 12, 2024

Main changes outside of test data are in tech_processors.py (removing > 2045) and debt_fraction_calc.py (specifying default tax credit case)

brtietz and others added 3 commits April 11, 2024 16:39
* Fix CRP as string bug

* rename self._crp to remove ambiguity
@brtietz brtietz requested a review from mikebannis April 12, 2024 19:30
@@ -126,7 +126,7 @@ def __init__(

Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed it in a past review, but tcc is missing from the __init__() docstring

Copy link
Collaborator

@mikebannis mikebannis left a comment

Choose a reason for hiding this comment

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

LGTM other than the missing docstring item

@@ -110,6 +110,7 @@ def __init__(
@param data_workbook_fname - name of workbook
@param case - financial case to run: 'Market' or 'R&D'
@param crp - capital recovery period: 20, 30, or 'TechLife'
@param tcc - tax credit case: 'ITC only' or 'PV PTC and Battery ITC' Only required for the PV plus battery technology.
Copy link
Collaborator

Choose a reason for hiding this comment

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

❤️

Copy link
Collaborator

Choose a reason for hiding this comment

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

Random question, what happens if someone sets the tcc for LBW or Nuclear?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would get passed through to the extractor and set in Excel, but wouldn't change the numbers at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should none be listed as a valid option given that behavior? Or does "only required for..." convey that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with that if you are. I don't think the additional complexity of preventing a user from using it incorrectly is worth it minor benefit.

Copy link
Collaborator

@mikebannis mikebannis Apr 12, 2024

Choose a reason for hiding this comment

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

In tcc : Optional[str] = None, the Optional officially allows the None. To make it a little more type safe you could do: tcc: Optional[ITC_ONLY_CASE| PTC_PLUS_ITC_CASE_PVB] = None

@brtietz brtietz merged commit cdeb16a into dev Apr 12, 2024
4 checks passed
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