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

Wind set backs #69

Merged
merged 15 commits into from
Dec 10, 2020
Merged

Wind set backs #69

merged 15 commits into from
Dec 10, 2020

Conversation

MRossol
Copy link
Collaborator

@MRossol MRossol commented Dec 8, 2020

Implementation of #65
TODO:
CLI
tests

@MRossol MRossol added the feature New feature or request label Dec 8, 2020
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.

Really nice code organization, great example of refactoring a notebook into something coherent. I do have a ton of questions / suggestions on how the docs could be improved. This is both because of my ignorance and also i think its crucial for posterity.


Parameters
----------
features_dir : str
Copy link
Member

Choose a reason for hiding this comment

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

great doc strings but incorrect description. probably elsewhere too.

What kind of files should these be? What extension does the code look for?

replace=replace)


class RailWindSetbacks(TransmissionWindSetbacks):
Copy link
Member

Choose a reason for hiding this comment

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

why are rail setbacks based on transmission while the others are not? Is it that you can't build turbines within x distance of structures and roads, but you can't build transmission lines within x distance of railways? If so, how are we going to use the rail wind setbacks for our non-existent transmission code? Or is this a grue problem?

Raster array of setbacks, ready to be written to the exclusions
.h5 file as a new exclusion layer
"""
setbacks = super().compute_setbacks(structure_fpath, groupby='State',
Copy link
Member

Choose a reason for hiding this comment

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

I'm of the opinion that subclass methods that only call the inherited method via super() with a kwarg change should not be subclass methods. It's just not worth the extra code due to additional complexity and maintenance. Why not remove this subclass method and call BaseWindSetbacks.compute_setbacks() with the kwarg in the subclass.run() methods?

Parameters
----------
excl_h5 : str
Path to .h5 file containing or to contain exclusion layers
Copy link
Member

Choose a reason for hiding this comment

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

This is where the new setbacks will be written? This is not a source file, correct? If it is a source file, what is being used from it?

setbacks, by default None
multiplier : int | float | str | None, optional
setback multiplier to use if wind regulations are not supplied,
if str, must one of {'high': 3, 'moderate': 1.1},
Copy link
Member

Choose a reason for hiding this comment

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

if str must be one of those keys? Kind of unclear. Maybe say that if str must be a key in cls.MULTIPLIERS?

Also what is the multiplier used for? I think based on our conversations and my fuzzy memory that this multiplies the max blade tip height to determine the setback, but that would be worthwhile to include in this description.

.format(cnty.iloc[0]['FIPS']))
tmp = gpd.sjoin(features, cnty,
how='inner', op='intersects')
tmp['geometry'] = tmp.buffer(setback)
Copy link
Member

Choose a reason for hiding this comment

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

ah this is where the magic happens! sweet.

.h5 file as a new exclusion layer
"""
if max_workers is None:
max_workers = os.cpu_count()
Copy link
Member

Choose a reason for hiding this comment

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

if you just do if max_workers == 1: serial else: parallel and send max_workers to the process pool directly with None/int you don't need to retrieve the os.cpu_count(). Obviously you can't do max_workers > 1 if max_workers is None but you can do max_workers == 1 if max_workers is None, Yay dynamic typing!

if regs_fpath:
if multiplier:
msg = ('A wind regulation .csv file was also provided and '
'will be used to determine setback multipliers!')
Copy link
Member

Choose a reason for hiding this comment

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

while this is clear in the source code, it would not be clear to a user what the csv was provided in addition to (the multiplier).

"""
trans = gpd.read_file(transmission_fpath)
mask = (~(trans['Proposed'] == 'Proposed')
& ~(trans['Undergroun'] == 'T'))
Copy link
Member

Choose a reason for hiding this comment

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

Undergroun a typo? (missing the d)

Also this is a super weird mask. Why not use != instead of ~==?


class TransmissionWindSetbacks(BaseWindSetbacks):
"""
Transmission Wind setbacks
Copy link
Member

Choose a reason for hiding this comment

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

without having to do a code diff can you explain how/why some of these transmission setbacks and methods are different than the structure setbacks?

@MRossol MRossol merged commit b1bfcf9 into master Dec 10, 2020
@MRossol MRossol deleted the wind_set_backs branch December 10, 2020 17:07
github-actions bot pushed a commit that referenced this pull request Dec 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants