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

Merge methods #154

Merged
merged 9 commits into from
Jul 5, 2022
Merged

Merge methods #154

merged 9 commits into from
Jul 5, 2022

Conversation

ppinchuk
Copy link
Collaborator

Added support for merging existing and extrapolated ordinances in one reVX invocation.
No new flag was needed - the user can now simply provide both a setback multiplier and a regulations file.

Sample comparisons shown below (with extreme cases to illustrate the functionality):

100
1000
3000

@ppinchuk ppinchuk linked an issue Jun 30, 2022 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2022

Codecov Report

Merging #154 (51f1f44) into main (71cb610) will increase coverage by 0.33%.
The diff coverage is 93.28%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #154      +/-   ##
==========================================
+ Coverage   80.59%   80.93%   +0.33%     
==========================================
  Files         110      110              
  Lines       11974    12075     +101     
==========================================
+ Hits         9651     9773     +122     
+ Misses       2323     2302      -21     
Flag Coverage Δ
unittests 80.93% <93.28%> (+0.33%) ⬆️

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

Impacted Files Coverage Δ
reVX/setbacks/wind_setbacks.py 63.04% <47.05%> (+0.45%) ⬆️
reVX/setbacks/base.py 89.53% <96.42%> (+2.21%) ⬆️
reVX/setbacks/parcel_setbacks.py 100.00% <100.00%> (ø)
reVX/setbacks/water_setbacks.py 100.00% <100.00%> (ø)
tests/test_setbacks.py 99.17% <100.00%> (+0.26%) ⬆️
reVX/handlers/database.py 0.00% <0.00%> (ø)
reVX/rpm/rpm_clusters.py 80.60% <0.00%> (+0.08%) ⬆️
reVX/plexos/plexos_plants.py 55.04% <0.00%> (+0.09%) ⬆️
... and 1 more

weights_calculation_upscale_factor=sf,
multiplier=100)
merged_layer = merged_setbacks.compute_setbacks(features_path,
max_workers=1)
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 a little confused on where the actual merge request happens... Like you're not actually requesting a merge of two exclusion layers but instead just combining the setback distance input with an explicit regulations file? Otherwise looks good to me!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, I think the title is a bit misleading (I essentially copied the issue request)

Anthony had said: "Either add a flag to handle all of this in one go e.g., model existing ordinances and apply [generic ordinances] elsewhere. Or have a new method that assumes you've already created the necessary datasets and just want to merge them (less ideal)."

He said the second option is less ideal so I went with the first idea (except there was no need for a new flag value)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Merging this so that Anthony can start using it, but let me know if you have any other questions/objections!

@ppinchuk ppinchuk merged commit e3a1ef0 into main Jul 5, 2022
@ppinchuk ppinchuk deleted the pp/merge_methods branch July 5, 2022 15:22
github-actions bot pushed a commit that referenced this pull request Jul 5, 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.

Method to Merge Existing and Extrapolated Ordinances
3 participants