Within capacity scaling, detect zones collisions and add ability to specify target area type#284
Merged
danielolsen merged 3 commits intodevelopfrom Sep 5, 2020
Merged
Conversation
ce4e853 to
3bf6cff
Compare
BainanXia
approved these changes
Sep 3, 2020
Collaborator
BainanXia
left a comment
There was a problem hiding this comment.
This is consistent with our discussion. Thanks!
jenhagg
reviewed
Sep 3, 2020
jenhagg
reviewed
Sep 3, 2020
jenhagg
reviewed
Sep 3, 2020
3bf6cff to
51a24a8
Compare
ce12387 to
f32a229
Compare
Contributor
Author
|
@jon-hagg I believe I've addressed all of your comments, please let me know if you think the latest version looks good. |
jenhagg
reviewed
Sep 4, 2020
| zone_sets = target_zones.values() | ||
| if len(set.union(*zone_sets)) != sum([len(t) for t in zone_sets]): | ||
| zone_sets_list = sum([list(z) for z in zone_sets], []) | ||
| duplicates = set([x for x in zone_sets_list if zone_sets_list.count(x) > 1]) |
Collaborator
There was a problem hiding this comment.
Couple ideas, mostly for readability
- I think we can define
duplicatesusing a set comprehension, e.g.{x for x in ...} - We could also define
zone_sets_list = [z for s in zone_sets for z in s]
Contributor
Author
There was a problem hiding this comment.
Good call! I updated those comprehensions for more readability, as well as the error_areas comprehension.
Collaborator
Thanks, looks good! I put a couple more comments if you're interested, but I'll go ahead and approve. |
jenhagg
approved these changes
Sep 4, 2020
f32a229 to
0f86c50
Compare
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
When attempting to refactor the quick bite script to calculate shortfall for future scenarios, @BainanXia and I found a bug: if there are two target names which both include the same zone, the demand from that zone will be arbitrarily assigned to one or the other. E.g. if we include
TexasandEl Paso,Texaswill by default look up the state of Texas which includes El Paso as one of its zones. We would like to detect this condition and return an error to the user to ask them to be more specific.This can create a new challenge, where in some scenarios when we say
Texaswe mean just the interconnection, with El Paso specified separately. In this case, we would like to be able to optionally specify anarea_typefor each target in the input CSV.What is the code doing
load_targets_from_csv(), we add another optional columnarea_typewith a default value ofnp.nan._make_zonename2target()we refactor the input parameters to expect a dataframe, not just a list of strings: this is so that we can check for a non-nan entry in thearea_typecolumn when looking up which zones are in each area, and if we find one then we pass that along to thearea_to_loadzone()lookup. This also allows us to remove theenforced_area_typeinput, since these are enforced target-wise via the dataframe. We also detect the case when there is a zone collision, and raise an error message.add_resource_data_to_targets()andadd_demand_to_targets(), we update the calls to_make_zonename2target().Validation
If we pass a CSV with both Texas and El Paso, we get an error message:
ValueError: zone in two target areas: El Paso in {'Texas', 'El Paso'}.If we correct this by specifying an
area_typeof 'interconnect' for Texas, then we get a new error, demonstrating pre-existing checks:ValueError: Targets do not cover all load zones. Missing: {'East Texas', 'Texas Panhandle'}. When we add these, or when we remove El Paso (since in this case we do want to include it with the rest of Texas) the process runs as before.Time to review
15 minutes. This is a relatively low priority, but will help us avoid mistakes when analyzing future scenarios.