Xmission/cost layers #122

mikebannis · 2021-07-22T17:20:04Z

No description provided.

MRossol

This is looking great. Some general pythonic comments and suggestions.

Two other asks:

Lets get some test data started, let me know if I can help getting the landuse layer added to the test 'ri_exclusions.h5' file
Can we remove the old modules/methods that this will replace?

reVX/least_cost_xmission/config/xmission_config.py

reVX/least_cost_xmission/cost_creator.py

MRossol · 2021-07-23T15:57:47Z

reVX/least_cost_xmission/cost_creator.py

-                 base_line_costs, iso_lookup, default_mults,
-                 excl_h5='/shared-projects/rev/exclusions/ATB_Exclusions.h5',
-                 slope_lyr='srtm_slope', nlcd_lyr='usa_mrlc_nlcd2011'):
+    def __init__(self, h5_fpath, iso_regions_fpath, atb_excl_h5, iso_lookup,


atb_excl_h5 is WAY to exact, this should just be excl_h5. Also I don't think we need this here, we should init the .h5 file from the iso_regions geotiff. We just need to make sure its profile matches the excl_h5 profile.

I'm ignoring the profile issue for a bit.

LOL, still waiting on Evan to fix that?

reVX/least_cost_xmission/cost_creator.py

MRossol · 2021-07-23T15:59:53Z

reVX/least_cost_xmission/cost_creator.py

+            # TODO - use self._profile instead of regions file
+            self._init_h5(h5_fpath, iso_regions_fpath)
+
+        self._tiff_dir = tiff_dir


This shouldn't be an attribute, we should ask people to explicitly give the geotiff path ONLY when saving to a geotfiff

Is the problem saving the tiff path as an instance attribute? Should create_geotiff() not be in build_cost_rasters() at all? I need tiffs for development purposes, but I can rip them out once this code is stable.

no, no, its that tiff_dir is ONLY used in create_geotiff, so it shouldn't be a class attribute. Instead it should be a kwarg in build_cost_rasters and then the actual geotiff path should be passed into create_geotiff.

What i would actually do is convert the save_geotiff kwargs into tiff_dir and use this logic:

if tiff_dir is not None: save_geotiff = True else: save_geotiff = False

What about

if tiff_dir: xcc.create_geotiff('mults.tif', tiff_dir, mults_arr)

Less code, though using to tiff_dir to set save_geotiff is more explicit.

the if statement logic is fine, but I don't follow the create_geotiff logic. Passing in the name and directory is silly, just pass in the full file path and create it from tiff_dir and mults.tif in the classmethod. Also when ever generating filenames be verbose: tie-line_cost_multipliers.tiff

See notes below.

MRossol · 2021-07-23T16:04:46Z

reVX/least_cost_xmission/cost_creator.py

+            if None
+        default_mults : dict
+            Multipliers for regions not specified in iso_mults_f.
+        save_to_h5 : bool


I still don't understand why we wouldn't want to write to the .h5 file... thats the whole point of the class, particularly for this classmethod I think the whole point of the workflow... What happens if you set both to False?

reVX/least_cost_xmission/cost_creator.py

MRossol · 2021-07-29T15:58:45Z

reVX/least_cost_xmission/config/xmission_config.py

+        """
+        super().__init__()
+
+        if power_to_voltage_fpath is None:


This is nit-picky, but I wonder if there is a way to reduce the number of if statements here. One thought is to build a dictionary like this:

defaults = {'power_to_voltage_fpath' : 'power_to_voltage', ...} for kwarg, name in defaults.items: self[name] = kwargs.get(kwarg, os.path.join(D_DIR, f'{name}.json'))

The only downside is that the init becomes which would be less verbose, but seeing as this is an internal handler the simplicity might be work it...

def __init__(self, **kwargs):

Noted. Let's revisit this once the code works.

MRossol · 2021-07-29T16:01:06Z

reVX/least_cost_xmission/cost_creator.py

+        self._profile['nodata'] = None
+        self._write_layer(self._excl_h5, layer_name, self._profile, data)
+
+    def create_geotiff(self, tiff_dir, geotiff_fpath, data):


Simplify this to be just:

def create_geotiff(self, geotiff_fpath, data):

There is no need to do an os.path.join inside this method.

Keeping os.path.join inside create_geotiff is much DRYer and keeps the code in build_cost_rasters() shorter. I'd prefer to save the tiff_dir as an attribute to further simply the function arguments. Thoughts?

I don't fully follow. Since this is a public method it seem much more straight forward to just request the full geotiff path and data to write (for anyone using this class that is not us). Then in the build_cost_rasters class method the tiff_dir is an input value so its a local variable, and we can create the custom file paths with os.path.join inside the if save_geotfif logic on lines 135 and 142.

reVX/least_cost_xmission/cost_creator.py

MRossol · 2021-07-29T16:03:51Z

reVX/least_cost_xmission/cost_creator.py

+        for i in indices:
+            mult_raster[i[0]] = i[1]
+
+        return mult_raster


Lets add a second class method, could be run, build, or create that would

run build_test_costs

add any extra layers using layers_to_h5

What do you mean by extra layers?

Transmission barrier layer and dist_to_coast layers I think are the only "other" layers we need. i.e none cost layers

MRossol · 2021-07-29T16:04:34Z

tests/test_x_mission_costs.py



 def test_land_use_multiplier():
    """ Test land use multiplier creation """
    lu_mults = {'forest': 1.63, 'wetland': 1.5}
    arr = np.array([[[0, 95, 90], [42, 41, 15]]])
-    cm = CostMultiplier()
-    out = cm._create_land_use_mult(arr, lu_mults)
+    xcc = XmissionCostCreator(INPUT_H5F, ISO_REGIONS_F, {})


whats up with the empty dict here?

iso region dict. It's not needed for the test.

Ah! Should we make that a kwarg then?

MRossol · 2021-07-29T16:05:18Z

@mikebannis getting super close! This is looking great. A few minor comments

MRossol · 2021-07-29T16:37:31Z

one more comment, lets split the test file into two:

test_xmission_cost_creator.py
test_xmission_least_cost.py

Simple Plant Builder CLI

MRossol · 2021-08-05T21:29:18Z

@mikebannis wanted to check in on this PR, would be nice to wrap this up before the next one...

mikebannis · 2021-08-05T21:38:44Z

Working on the path finding code. I'll wrap this up tonight or tmw

MRossol · 2021-08-05T21:39:46Z

No rush just a gentle nudge before we both forget what we were thinking, so definitely tomorrow, you don't get paid enough to work tonight...

MRossol · 2021-08-06T21:06:43Z

reVX/least_cost_xmission/cost_creator.py

+                                   costs_arr)
+
+    @classmethod
+    def build_test_costs(cls, h5_fpath, iso_regions_fpath, input_h5_fpath,


This isn't useful outside of the tests, we should just move this code to the tests as a function that can be run if needed.

MRossol · 2021-08-06T21:07:35Z

reVX/least_cost_xmission/cost_creator.py

+
+        # _write_geotiff sets nodata to the max for the dtype. For float32 this
+        # then breaks JSONifying the profile when writing h5s. Reset it.
+        self._profile['nodata'] = nodata


Why is nodata pulled out and then added back to the profiel? I dont' see it being used in this function, are the above comments supposed to be a TODO?

fix docstring in excl converter

add tests

mikebannis · 2021-08-10T22:11:37Z

reVX/least_cost_xmission/config/__init__.py

@@ -4,7 +4,7 @@
 """
 import os

-from .xmission_config import XmissionConfig
+from reVX.least_cost_xmission.config.xmission_config import XmissionConfig


Out of curiosity, why the absolute versus relative import? It seems the relative import is less work if the folder structure changes.

No good reason beyond Grant's preference that we started following in reV and has bleed over to reVX. We have run into some strange import issues in the past but I think that was mainly noobness + python 3.6 being weird.

mikebannis · 2021-08-10T22:13:55Z

reVX/least_cost_xmission/config/config.json

@@ -0,0 +1 @@
+{"iso_lookup": {"MISO": 3, "SCE": 2, "Southeast": 4, "TEPPC": 1}, "min_power_classes": {"100MW": 0, "200MW": 100, "400MW": 200, "1000MW": 400}, "new_substation_costs": {"MISO": {"138": 7800000, "230": 9700000, "500": 20300000, "69": 5700000}, "SCE": {"138": 8870000, "230": 17525000, "500": 37017000, "69": 3975000}, "Southeast": {"138": 4692000, "230": 5732000, "500": 20364000, "69": 3064000}, "TEPPC": {"138": 5541000, "230": 9171000, "500": 15489000, "69": 3498000}}, "power_classes": {"1000MW": 1500, "100MW": 102, "200MW": 205, "400MW": 400}, "transformer_costs": {"115": {"115": 5230, "138": 4260, "161": 4480, "230": 4970, "345": 5790, "500": 7110, "69": 3840}, "138": {"115": 4260, "138": 5790, "161": 4720, "230": 4970, "345": 5790, "500": 7110, "69": 4260}, "161": {"115": 4480, "138": 4720, "161": 6420, "230": 5230, "345": 6100, "500": 7490, "69": 4480}, "230": {"115": 4970, "138": 4970, "161": 5230, "230": 7110, "345": 6100, "500": 7490, "69": 4970}, "345": {"115": 5790, "138": 5790, "161": 6100, "230": 6100, "345": 8670, "500": 7860, "69": 6100}, "500": {"115": 7110, "138": 7110, "161": 7490, "230": 7490, "345": 7860, "500": 11610, "69": 7860}, "69": {"115": 3840, "138": 4260, "161": 4480, "230": 4970, "345": 6100, "500": 7860, "69": 4720}}, "power_to_voltage": {"102": 69, "1500": 500, "171": 115, "205": 138, "240": 161, "343": 230, "400": 230, "68": 46, "750": 345}, "upgrade_substation_costs": {"MISO": {"138": 1600000, "230": 2200000, "500": 5300000, "69": 1100000}, "SCE": {"138": 1117000, "230": 3975000, "500": 9665000, "69": 767000}, "Southeast": {"138": 1179000, "230": 2210000, "500": 8340000, "69": 917000}, "TEPPC": {"138": 2198000, "230": 5500000, "500": 10236000, "69": 1352000}}, "iso_multipliers": [{"iso": "TEPPC", "land_use": {"cropland": 1, "forest": 2.25, "suburban": 1.3, "urban": 1.59, "wetland": 1.2}, "slope": {"hill_mult": 1.4, "hill_slope": 2, "mtn_mult": 1.75, "mtn_slope": 8}}, {"iso": "SCE", "land_use": {"cropland": 1, "forest": 3, "suburban": 2, "urban": 3, "wetland": 2}, "slope": {"hill_mult": 1.5, "hill_slope": 2, "mtn_mult": 2, "mtn_slope": 8}}, {"iso": "MISO", "land_use": {"cropland": 1.03, "forest": 1.2, "suburban": 1.08, "urban": 1.2, "wetland": 1.8}, "slope": {"hill_mult": 1.1, "hill_slope": 2, "mtn_mult": 1.21, "mtn_slope": 8}}, {"iso": "Southeast", "land_use": {"cropland": 1, "forest": 1.47, "suburban": 1.75, "urban": 1.12, "wetland": 1.33}, "slope": {"hill_mult": 1.2, "hill_slope": 2, "mtn_mult": 1.6, "mtn_slope": 8}}], "base_line_costs": {"MISO": {"102": 1255000, "1500": 2787000, "205": 1446000, "400": 1695000}, "SCE": {"102": 1524000, "1500": 4777000, "205": 2084000, "400": 3034000}, "Southeast": {"102": 819000, "1500": 4056000, "205": 984000, "400": 1568000}, "TEPPC": {"102": 984000, "1500": 2248000, "205": 1180000, "400": 1570000}}, "nlcd_land_use_classes": {"cropland": [80, 81], "forest": [41, 42, 43], "wetland": [90, 95], "suburban": [21, 22, 23], "urban": [24]}}


Is it safe to assume github actions will format that?

Yeah I did a no-verify commit just to get up before calling it a day. Going to finish it up / clean it up now.

turn off geotiff checker

codecov-commenter · 2021-08-11T03:25:53Z

Codecov Report

Merging #122 (2894b8c) into mjb/feat/x-mission-path (adab7d9) will increase coverage by 0.06%.
The diff coverage is 78.52%.

@@                     Coverage Diff                     @@
##           mjb/feat/x-mission-path     #122      +/-   ##
===========================================================
+ Coverage                    79.63%   79.69%   +0.06%     
===========================================================
  Files                           93      102       +9     
  Lines                         9529    10095     +566     
===========================================================
+ Hits                          7588     8045     +457     
- Misses                        1941     2050     +109

Flag	Coverage Δ
unittests	`79.69% <78.52%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
reVX/handlers/xmission_cost_layers.py	`0.00% <0.00%> (ø)`
reVX/least_cost_xmission/least_cost_xmission.py	`0.00% <0.00%> (ø)`
reVX/least_cost_xmission/utilities.py	`0.00% <0.00%> (ø)`
reVX/utilities/exclusions_converter.py	`76.99% <ø> (ø)`
setup.py	`0.00% <ø> (ø)`
reVX/handlers/geotiff.py	`71.72% <33.33%> (+6.23%)`	⬆️
reVX/least_cost_xmission/config/xmission_config.py	`50.00% <50.00%> (ø)`
reVX/least_cost_xmission/cost_creator_cli.py	`68.53% <68.53%> (ø)`
reVX/plexos/simple_plant_builder_cli.py	`84.37% <84.37%> (ø)`
reVX/least_cost_xmission/cost_creator.py	`84.72% <84.72%> (ø)`
... and 34 more

mikebannis · 2021-08-12T22:56:35Z

I merged xmission/cost_layers into xmission/cost_paths on my computer. Should we just close this and delete the PR?
At this point I'm tempted to just delete mjb/feat/x...

MRossol · 2021-08-13T15:06:30Z

Fine by me, i'll close this PR with out merging into mjb/.... and agreed lets delete mjb/... I'll delete it here so you just have to delete your remote branch

mikebannis requested a review from MRossol July 22, 2021 17:20

MRossol requested changes Jul 22, 2021

View reviewed changes

MRossol requested changes Jul 23, 2021

View reviewed changes

mikebannis force-pushed the xmission/cost_layers branch 3 times, most recently from 836292c to b7b3b6b Compare July 27, 2021 19:56

MRossol self-requested a review July 29, 2021 15:52

MRossol requested changes Jul 29, 2021

View reviewed changes

MRossol and others added 4 commits August 3, 2021 13:11

use atol in reeds test

b93a6c6

set RTOL to 0.01

731b740

add CLI and test for simple plant builder

a538e37

Merge pull request #124 from NREL/simple_plant_cli

40228df

Simple Plant Builder CLI

add simple plant builder CLI to docs

2600057

MRossol requested changes Aug 6, 2021

View reviewed changes

mikebannis added 11 commits August 10, 2021 12:29

Rasterize ISO regions

b21e796

Create cost multiplier raster

b684998

Create cost multiplier raster

5ae75d6

Calculate mcps from SC points to substations

e2b22e2

Include t-lines in minimum cost paths

9493712

Save new transformer costs config

8a52740

Include json config files for multipliers and costs

e4f6cc1

User file handlers for loading most data

803032f

Remove " from numbers in config JSONs

d168c20

Add classes for loading all data and processing multiple SC pts

5e0b7a2

Include transformer costs in new tie-line costs

aa1e796

mikebannis and others added 19 commits August 10, 2021 12:29

Include connection costs for sinks and load centers

1d7d915

Fix tests

265bd4c

Use non-sequential chunking to improve run times

0ff364d

start refactor

450c0ff

start cost_layer class

58c15bb

start handler

b753296

add notes to XmissionCostCreator

f5f4a22

fix docstring in excl converter

add transmission costs layer handler

fea775e

fix methods

91abc22

make error message more verbose

262419f

fix geotiff getter

dd51eba

add tests

Reimplement cost layer code in new class structure

7a3b766

Reimplementation runs and can save to h5

62bbb2b

Refactor tie-line costs per PR review

4e8d1f3

Add ISO regions and nlcd to RI test data

457f13b

Fix RI nlcd tiff

b3991c3

Fix doc strings

fbcfd69

Add RI NLCD to h5 test data file

193f6b4

Additional tests for tie-line cost calcs

b4d4242

MRossol force-pushed the xmission/cost_layers branch from a6c8fc5 to b4d4242 Compare August 10, 2021 18:29

cleanup cost_creator and start CLI

e172981

mikebannis commented Aug 10, 2021

View reviewed changes

add CLI and CLI test

2894b8c

turn off geotiff checker

parse cli inputs from json

6f74dad

MRossol closed this Aug 13, 2021

MRossol deleted the xmission/cost_layers branch August 13, 2021 15:18

		@@ -0,0 +1 @@
		{"iso_lookup": {"MISO": 3, "SCE": 2, "Southeast": 4, "TEPPC": 1}, "min_power_classes": {"100MW": 0, "200MW": 100, "400MW": 200, "1000MW": 400}, "new_substation_costs": {"MISO": {"138": 7800000, "230": 9700000, "500": 20300000, "69": 5700000}, "SCE": {"138": 8870000, "230": 17525000, "500": 37017000, "69": 3975000}, "Southeast": {"138": 4692000, "230": 5732000, "500": 20364000, "69": 3064000}, "TEPPC": {"138": 5541000, "230": 9171000, "500": 15489000, "69": 3498000}}, "power_classes": {"1000MW": 1500, "100MW": 102, "200MW": 205, "400MW": 400}, "transformer_costs": {"115": {"115": 5230, "138": 4260, "161": 4480, "230": 4970, "345": 5790, "500": 7110, "69": 3840}, "138": {"115": 4260, "138": 5790, "161": 4720, "230": 4970, "345": 5790, "500": 7110, "69": 4260}, "161": {"115": 4480, "138": 4720, "161": 6420, "230": 5230, "345": 6100, "500": 7490, "69": 4480}, "230": {"115": 4970, "138": 4970, "161": 5230, "230": 7110, "345": 6100, "500": 7490, "69": 4970}, "345": {"115": 5790, "138": 5790, "161": 6100, "230": 6100, "345": 8670, "500": 7860, "69": 6100}, "500": {"115": 7110, "138": 7110, "161": 7490, "230": 7490, "345": 7860, "500": 11610, "69": 7860}, "69": {"115": 3840, "138": 4260, "161": 4480, "230": 4970, "345": 6100, "500": 7860, "69": 4720}}, "power_to_voltage": {"102": 69, "1500": 500, "171": 115, "205": 138, "240": 161, "343": 230, "400": 230, "68": 46, "750": 345}, "upgrade_substation_costs": {"MISO": {"138": 1600000, "230": 2200000, "500": 5300000, "69": 1100000}, "SCE": {"138": 1117000, "230": 3975000, "500": 9665000, "69": 767000}, "Southeast": {"138": 1179000, "230": 2210000, "500": 8340000, "69": 917000}, "TEPPC": {"138": 2198000, "230": 5500000, "500": 10236000, "69": 1352000}}, "iso_multipliers": [{"iso": "TEPPC", "land_use": {"cropland": 1, "forest": 2.25, "suburban": 1.3, "urban": 1.59, "wetland": 1.2}, "slope": {"hill_mult": 1.4, "hill_slope": 2, "mtn_mult": 1.75, "mtn_slope": 8}}, {"iso": "SCE", "land_use": {"cropland": 1, "forest": 3, "suburban": 2, "urban": 3, "wetland": 2}, "slope": {"hill_mult": 1.5, "hill_slope": 2, "mtn_mult": 2, "mtn_slope": 8}}, {"iso": "MISO", "land_use": {"cropland": 1.03, "forest": 1.2, "suburban": 1.08, "urban": 1.2, "wetland": 1.8}, "slope": {"hill_mult": 1.1, "hill_slope": 2, "mtn_mult": 1.21, "mtn_slope": 8}}, {"iso": "Southeast", "land_use": {"cropland": 1, "forest": 1.47, "suburban": 1.75, "urban": 1.12, "wetland": 1.33}, "slope": {"hill_mult": 1.2, "hill_slope": 2, "mtn_mult": 1.6, "mtn_slope": 8}}], "base_line_costs": {"MISO": {"102": 1255000, "1500": 2787000, "205": 1446000, "400": 1695000}, "SCE": {"102": 1524000, "1500": 4777000, "205": 2084000, "400": 3034000}, "Southeast": {"102": 819000, "1500": 4056000, "205": 984000, "400": 1568000}, "TEPPC": {"102": 984000, "1500": 2248000, "205": 1180000, "400": 1570000}}, "nlcd_land_use_classes": {"cropland": [80, 81], "forest": [41, 42, 43], "wetland": [90, 95], "suburban": [21, 22, 23], "urban": [24]}}

Xmission/cost layers #122

Xmission/cost layers #122

Conversation

mikebannis commented Jul 22, 2021

MRossol left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MRossol commented Jul 29, 2021

MRossol commented Jul 29, 2021

MRossol commented Aug 5, 2021

mikebannis commented Aug 5, 2021

MRossol commented Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 11, 2021 • edited Loading

Codecov Report

mikebannis commented Aug 12, 2021

MRossol commented Aug 13, 2021

MRossol left a comment •

edited

Loading

codecov-commenter commented Aug 11, 2021 •

edited

Loading