Skip to content

Improve Grid and Scenario init flexibility#323

Merged
danielolsen merged 2 commits intodevelopfrom
daniel/flexible_inits
Oct 29, 2020
Merged

Improve Grid and Scenario init flexibility#323
danielolsen merged 2 commits intodevelopfrom
daniel/flexible_inits

Conversation

@danielolsen
Copy link
Copy Markdown
Contributor

Purpose

Allow greater flexibility and reduce syntax for common inits.

What the code is doing

  • Allows Grids to be instantiated with a string, instead of requiring that single-interconnect grids still be wrapped in a list.
  • Allows Scenarios to be instantiated with an int, instead of requiring an int in str form.

Testing

We can make use of the Grid equality check to test.

>>> from powersimdata.scenario.scenario import Scenario
>>> from powersimdata.input.grid import Grid
>>> Grid(["Eastern"]) == Grid("Eastern")
True
>>> Scenario(1270).state.get_grid() == Scenario("1270").state.get_grid()
SCENARIO: Julia | USA2030HVDC_Design3_OB1_Mesh500x38

--> State
analyze
--> Loading grid
--> Loading ct
SCENARIO: Julia | USA2030HVDC_Design3_OB1_Mesh500x38

--> State
analyze
--> Loading grid
--> Loading ct
True

Time estimate

5 minutes

@BainanXia
Copy link
Copy Markdown
Collaborator

Haha, this makes life easier when doing quick checks 👍

Comment thread powersimdata/scenario/scenario.py Outdated
if isinstance(descriptor, int):
descriptor = str(descriptor)
if not isinstance(descriptor, str):
raise TypeError("Descriptor must be a string")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we change the error message to: descriptor must be a int/str ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

raise ValueError(f"Engine must be one of {','.join(supported_engines)}")

if isinstance(interconnect, str):
interconnect = [interconnect]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe we can add:

if not isinstance(interconnect, list):
    raise TypeError("interconnect must be a str/list for a single-interconnect and a list for multiple")

and update the docstring accordingly. How does it sound

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"interconnect must be a str or list of str"? "single" and "multiple" doesn't really track for "USA", which is a single string representing three interconnects.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess it would be nice to check the value of interconnect. We can use the check_interconnect function in the powersimdata.scenario.helpers module. It is probably out of the scope of the PR but since we are dealing with arguments it would be sweet.

Copy link
Copy Markdown
Contributor Author

@danielolsen danielolsen Oct 29, 2020

Choose a reason for hiding this comment

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

We already have a check_interconnect in powersimdata.network.usa_tamu.usa_tamu_model for the TAMU grids, this function gets used when we create scenarios so it should also have been checked by the time we're loading any grids/scenarios from FromREISE or FromREISEjl grids, right?

The version in powersimdata.scenario.helpers seems like it has some hardcoded values that apply to USA/TAMU but would not be valid once we expand our geographical reach.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Indeed. I forgot about that one!

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think at some we will need to set an attributes in the Grid class that encloses the model we use. Right now, it will always be usa_tamu. Each model will then have its own check_interconnect and other meaningful methods and a higher level function will use the right ones in the scenario framework depending on the model.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I.e. #84?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That said, I am not sure that we need a new model in the ScenarioList.csv file. We could just pack a new str in the case.mat and we retrieve it when we build the grid back from the grid.mat. Because I really like to re-process the old MAT-file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Error message updated.

@danielolsen danielolsen force-pushed the daniel/flexible_inits branch from e55b5bf to 4e77e76 Compare October 29, 2020 17:54
Copy link
Copy Markdown
Collaborator

@rouille rouille left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks

@danielolsen danielolsen merged commit 3ee57ca into develop Oct 29, 2020
@danielolsen danielolsen deleted the daniel/flexible_inits branch October 29, 2020 18:32
@ahurli ahurli mentioned this pull request Mar 11, 2021
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.

3 participants