-
Notifications
You must be signed in to change notification settings - Fork 6
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
Techsub revamp #38
Techsub revamp #38
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @samgdotson! Thanks for submitting this PR. I appreciate moving some of the code into the parent class to remove repeated code. I have a few questions about parts of the code. Do you have CI set up for this repository? If you don't I will create an issue to get that up so that reviewers can see if the tests are passing.
Thanks @abachma2, I think I’ve answered your questions. The CI is performed by github actions and can be viewed under the “checks” tab. |
Thanks for pointing that out. I missed the checked passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @samgdotson, great work on this PR. I only have a couple of comments.
super().__init__(technology_type=technology_type, | ||
technology_category=technology_category, | ||
*args, **kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So much cleaner! Good work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I didn't notice any other issues, just had one minor question pop up.
@yardasol @abachma2 @LukeSeifert, thanks for your comments and suggestions! I think I've addressed all of them in some capacity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing our comments. Once the confusion about the power_units
in osier/models/capacity_expansion.py
is cleared up I will approve.
I will approve this PR. I will let @yardasol merge when he approves it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work @samgdotson
This PR
land_intensity
andco2_rate
.validate_quantity
now handles array_types as well as constant_types.