-
Notifications
You must be signed in to change notification settings - Fork 24
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
Doc updates #421
Doc updates #421
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #421 +/- ##
==========================================
+ Coverage 86.87% 86.91% +0.04%
==========================================
Files 122 122
Lines 16810 16823 +13
==========================================
+ Hits 14603 14622 +19
+ Misses 2207 2201 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
reV/supply_curve/sc_aggregation.py
Outdated
"nodata_value": -1 | ||
}, | ||
"partial_setback": { | ||
"use_as_weights": 1 |
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.
This should probably be a boolean True to avoid confusion with a weight multiplier of 1.
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 catch; I updated to True
reV/supply_curve/sc_aggregation.py
Outdated
|
||
excl_dict = { | ||
"typical_exclusion": { | ||
"exclude_values": 1, |
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.
I think this docstring pops up one more place so comment is general... Can we have this not be 0 or 1 to avoid confusion with a boolean option? I also have a comment on the confusion of a boolean below.
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.
Yes, great point. Updated in both places.
reV/supply_curve/sc_aggregation.py
Outdated
"include_range": [0, 20] | ||
}, | ||
"developable_land": { | ||
"force_include_values": 1 |
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.
Same here, can we make this 42 or something so its not confused with a bool?
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.
Yes, thank you. Updated to 42 ;)
Awesome comments, thank you! The linter error should stop showing up on the main branch |
Some minor documentation additions (in particular to the
excl_dict
doc)