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

Feat/offshore lcp #150

Merged
merged 41 commits into from
Dec 13, 2022
Merged

Feat/offshore lcp #150

merged 41 commits into from
Dec 13, 2022

Conversation

mikebannis
Copy link
Collaborator

Not done yet and need to do a lot of linting. Just leaving this here as a heads up and to see if there are any grievous issues I need to fix.

@codecov-commenter
Copy link

codecov-commenter commented Jun 9, 2022

Codecov Report

Merging #150 (2d2e27c) into main (3a3694a) will decrease coverage by 1.99%.
The diff coverage is 27.93%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #150      +/-   ##
==========================================
- Coverage   82.55%   80.56%   -2.00%     
==========================================
  Files         117      118       +1     
  Lines       12820    13225     +405     
==========================================
+ Hits        10584    10655      +71     
- Misses       2236     2570     +334     
Flag Coverage Δ
unittests 80.56% <27.93%> (-2.00%) ⬇️

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

Impacted Files Coverage Δ
reVX/least_cost_xmission/aoswt_utilities.py 0.00% <0.00%> (ø)
...eVX/least_cost_xmission/least_cost_xmission_cli.py 52.00% <38.04%> (-19.58%) ⬇️
reVX/least_cost_xmission/cost_creator.py 88.00% <70.00%> (-0.60%) ⬇️
reVX/least_cost_xmission/trans_cap_costs.py 81.32% <70.83%> (-0.98%) ⬇️
reVX/least_cost_xmission/least_cost_xmission.py 90.33% <86.00%> (-0.74%) ⬇️
reVX/config/least_cost_xmission.py 97.60% <89.65%> (-2.41%) ⬇️
reVX/least_cost_xmission/least_cost_paths.py 92.64% <100.00%> (ø)
tests/test_xmission_least_cost.py 91.93% <100.00%> (ø)
reVX/setbacks/parcel_setbacks.py 97.43% <0.00%> (-2.57%) ⬇️
reVX/handlers/database.py 0.00% <0.00%> (ø)
... and 2 more

Copy link
Member

@grantbuster grantbuster left a comment

Choose a reason for hiding this comment

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

Most of this looks good but its hard to understand exactly what your goals are, i'd appreciate a quick call just to get the rundown of what all this work is about.

self._name = self.get('name')
else:
# name defaults to base directory name
self._name = os.path.basename(os.path.normpath(self.dirout))
Copy link
Member

Choose a reason for hiding this comment

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

you should do something like this:

default_name = os.path.basename(os.path.normpath(self.dirout))
self._name = self.get('name', default_name)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,604 @@
"""
Various utility functions to prep data for AOSWT processing.
Copy link
Member

Choose a reason for hiding this comment

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

Would this module not work for the pacific? Why not?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like Mike was in the process of working on this (see quote form him below), but the demand for this work dropped off. I propose to shelve this question until demand for this work arises once more

TODO:

  • When saving paths, also export to CSV
  • calculate % path distance offshore vs land and save to dist_offshore_frac column
  • change README to rst
  • remove AOSWT from naming when appropriate and make ocean agnostic
  • Update linter config (done)
  • Make 500kV default voltage for fake t-lines

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The AOSWT code would really work for determining paths anywhere within the reV raster, including on land.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we rename the file?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The code was written for AOSWT (Atlantic off-shore wind transmission) and has only been used for AOSWT so far. Naming it after it provides some history, assuming the user knows what AOSWT is. Keeping it named AOSWT until it's used for something else makes sense to me. That said, I won't be offended if you decide to change it.

# Append some fake values to make the LCP code happy
lines['ac_cap'] = 9999999
lines['category'] = 'Substation'
lines['voltage'] = 69
Copy link
Member

Choose a reason for hiding this comment

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

maybe make the fake voltage 0 or 1e9?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The substation needs a transmission line with adequate voltage for the code to connect to it. I arbitrarily chose 69kV as a standard voltage. 1e9 may work as well but I would need to check it.

Combine layers to create composite friction and barrier rasters. Merge
with existing land cost and barriers and save to h5.
"""
OS_FRICTION_FNAME = 'os_friction.tif'
Copy link
Member

Choose a reason for hiding this comment

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

What is OS? Offshore? Onshore is also OS?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Renamed to "Offshore" for clarity

save_tiff : bool, optional
Save composite friction to tiff if true
"""
print('Loading friction layers')
Copy link
Member

Choose a reason for hiding this comment

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

Please convert all print statements to logger.info() statements

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done


def _load_layer(self, f, val, verbose=False):
"""
Load a layer from a tiff and set appropriate cells to 1
Copy link
Member

Choose a reason for hiding this comment

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

Can you only handle tiff inputs? Should we support our collection of h5 exclusion layers?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does not seem to be major demand for this request, so I propose we shelve it. In the immediate future, anyone looking to use this function with an h5 exclusion layer can use the $ exclusions layers-from-h5 command line utility to extract h5 layers as tiff.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A little context: I'm not very good at using h5s and I frequently load raster and vector data into QGIS for using basemaps and zooming. QGIS doesn't speak h5. Tiffs are also much smaller than h5 files and easier to copy to and from eagle. So that's why some of this code is setup for using tiffs. I believe all the input files have already been converted to h5 files. Switching over to a pure h5 workflow should not be an issue.

@mikebannis
Copy link
Collaborator Author

TODO:

  • When saving paths, also export to CSV
  • calculate % path distance offshore vs land and save to dist_offshore_frac column
  • change README to rst
  • remove AOSWT from naming when appropriate and make ocean agnostic
  • Update linter config (done)
  • Make 500kV default voltage for fake t-lines

@ppinchuk
Copy link
Collaborator

ppinchuk commented Dec 6, 2022

@mikebannis Are there any plans to finalize this? I will be working on some changes to LCP next week, so I'd like to know if I need to work around this PR or not

@ppinchuk
Copy link
Collaborator

ppinchuk commented Dec 6, 2022

After conversation with Mike, I will attempt to do bare minimum to make this PR mergeable

grantbuster
grantbuster previously approved these changes Dec 12, 2022
@ppinchuk ppinchuk merged commit dec8cf3 into main Dec 13, 2022
@ppinchuk ppinchuk deleted the feat/offshore_lcp branch December 13, 2022 06:19
github-actions bot pushed a commit that referenced this pull request Dec 13, 2022
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.

None yet

4 participants