-
Notifications
You must be signed in to change notification settings - Fork 9
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
Mjb/feat/x mission path #115
Conversation
ecaa509
to
d5848df
Compare
cb2ff6b
to
13a4b7f
Compare
Codecov Report
@@ Coverage Diff @@
## main #115 +/- ##
==========================================
- Coverage 80.00% 77.94% -2.06%
==========================================
Files 91 97 +6
Lines 9306 9852 +546
==========================================
+ Hits 7445 7679 +234
- Misses 1861 2173 +312
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
just a start...
meta = src.meta | ||
for feat in src: | ||
feats.append(feat) | ||
print(f'Loaded {len(feats)} features from {in_shp}') |
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.
Use logging instead of print statements. The best practices for logging are:
- setup a logger at the module (file) level using the module name:
logger = logging.getLogger(__name__)
- Have the user configure the logger, the only exception here is in the CLI where we setup the loggers to either log to the stream or the stream + a file.
dst.writerecords(new_feats) | ||
|
||
|
||
def main(): |
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.
Its frowned upon to have scripts inside a package. Turn this into a CLI if you need to execute it, other wise create the script elsewhere.
import sys | ||
|
||
|
||
def update_substations(in_shp, out_shp, template): |
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.
Why are we doing this before hand? Isn't this going to depend on the supply curve resolution? If so we should merge this functionality into the main class and have it run on the fly.
METERS_IN_MILE = 1609.344 | ||
|
||
|
||
def buildCostRasters(iso_regions_f, nlcd_f, slope_f, exclusions_f, template_f, |
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 be a class method of CostMultiplier not a stand alone function.
TODO | ||
|
||
""" | ||
print('Loading all files') |
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.
See above about print statements...
logger.debug('Determining voltages for substations, this will take ' | ||
'a while') | ||
self.subs['temp_volts'] = self.subs.apply(self._get_volts, axis=1) | ||
volts = self.subs.temp_volts.str.split('/', expand=True) |
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 try to avoid the parameter notation in pandas/geopandas as it is confusing. Is temp_volts a method? Oh wait its a column. Lets just use column notation.
logger = logging.getLogger(__name__) | ||
|
||
|
||
class BlockedTransFeature(Exception): |
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.
We have a module for custom exceptions:
https://github.com/NREL/reVX/blob/main/reVX/utilities/exceptions.py
Serial process tie line costs for Supply Curve points | ||
|
||
""" | ||
def __init__(self, capacity_class='100MW', resolution=128, n=10): |
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.
capacity_class shouldn't be a kwarg
pass | ||
|
||
|
||
class TransmissionCost: |
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.
Whats the point of this class? It seems like its a glorified dictionary
@@ -0,0 +1,106 @@ | |||
import rasterio as rio |
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.
Lets move this method to reVX/utilities/ sub-package
Also add multiplier cost for water
933f9b2
to
d0c0a6e
Compare
Closing for refactor |
No description provided.