-
Notifications
You must be signed in to change notification settings - Fork 40
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: add functionality to load Grid objects with HIFLD grid model #566
Conversation
I know we made a mistake to use Texas in place of ERCOT for the TAMU grid but should right our mistake for HIFLD? We have a lot of code that check/analyze/plot interconnect(s), I presume these expect Texas. |
I think any code that's not model-specific should look to the grid model for the appropriate interconnections. If our check/analyze/plot code isn't working, then we should change it to look for the right information in the right place (and make sure that information is there). We'll need this sooner or later, either for the new USA grid model or for models of other grids (e.g. Europe). |
9acf63c
to
d78a404
Compare
After tweaking the output plant CSV a bit more within Breakthrough-Energy/PreREISE#236 (adding type, adding heat rate columns, translating to PowerSimData naming expectations), and updating the placeholders within in |
The latest commits add the missing constants required to fully populate the >>> from powersimdata.network.model import ModelImmutables
>>> mi_tamu = ModelImmutables("usa_tamu")
>>> mi_hifld = ModelImmutables("hifld")
>>> mi_tamu.plants.keys() == mi_hifld.plants.keys()
True
>>> mi_tamu.zones.keys() == mi_hifld.zones.keys()
True
>>> mi_tamu.storage.keys() == mi_hifld.storage.keys()
True
Therefore, this is now ready for a full review. |
fe1e27f
to
ca9892c
Compare
|
||
|
||
class HIFLD(AbstractGrid): | ||
def __init__(self, interconnect): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could move all the instance methods to AbstractGrid
, then just declare:
class HIFLD(AbstractGrid):
pass
Same for the TAMU grid. Edit: I guess the __file__
would have to be passed to super
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense. Is that still an abstract class then. It would look like a Parent class. Do we need to rename it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, didn't see this one so replied below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we do within the child HIFLD
and TAMU
classes:
def __init__(self, interconnect):
self.top_dirname = os.path.dirname(__file__)
self.interconnect = check_and_format_interconnect(interconnect)
super().__init__()
and then within the parent AbstractGrid
class:
def __init__(self):
self._set_data_loc()
self._build_network()
def _set_data_loc(self):
"""Sets data location
:raises IOError: if directory does not exist.
"""
data_loc = os.path.join(self.top_dirname, "data")
if os.path.isdir(data_loc) is False:
raise IOError("%s directory not found" % data_loc)
else:
self.data_loc = data_loc
And within the ScenarioGrid
class:
def __init__(self, filename):
self.filename = filename
super().__init__()
def _set_data_loc(self):
"""Sets data location.
:raises FileNotFoundError: if file does not exist.
"""
if os.path.isfile(self.filename) is False:
raise FileNotFoundError("%s file not found" % self.filename)
else:
self.data_loc = filename
The result: the children set their pre-init information (self.top_dirname
for TAMU
and HIFLD
, from __file__
; self.filename
for ScenarioGrid
), and then the parent always calls _set_data_loc
and _build_network
, which are tweaked to take only self
as an input and look to the appropriate attributes to get the directory or filename that they need to work with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved things around and everything appears to be working, including loading a grid from a previously-run Scenario.
"solar_curtailment": "Solar Curtailment", | ||
"wind_curtailment": "Wind Curtailment", | ||
"wind_offshore_curtailment": "Offshore Wind Curtailment", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that PostREISE plotting functions rely on this dictionary and it is convenient to group generator type and curtailment but it is ugly. Curtailment color label/color should be moved in a dedicated dictionary. This completely out of the scope of this PR.
"solar_curtailment": "xkcd:grey", | ||
"wind_curtailment": "xkcd:grey", | ||
"wind_offshore_curtailment": "xkcd:grey", | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here curtailment is not a generator type
|
||
|
||
class HIFLD(AbstractGrid): | ||
def __init__(self, interconnect): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense. Is that still an abstract class then. It would look like a Parent class. Do we need to rename it?
An abstract class can have some behavior defined. Typically it would also declare abstract methods, to be implemented by derived classes. So if there are only "concrete" methods, a rename could make sense, e.g. |
edc67ff
to
292e70d
Compare
292e70d
to
4595c56
Compare
Pull Request doc
Purpose
Allows the user to instantiate HIFLD grid objects by adding the necessary folder structure and sub-modules. Closes #551.
What the code is doing
powersimdata.network.hifld.model
: defining an HIFLD class, and populating it with necessary methods. Most of this code is copied directly frompowersimdata.network.usa_tamu.model
powersimdata.input.grid
: adding"hifld"
as an allowable model/source input, and instantiating anHIFLD
object as appropriate.powersimdata.network.model
: adding"hifld"
as an acceptable model for instantiating aModelImmutables
object.__init__.py
files as necessary withinpowersimdata.network.hifld.constants
. These will be filled in before this PR gets promoted out of draft stage.Testing
Current testing relies on Breakthrough-Energy/PreREISE#236
and Breakthrough-Energy/PreREISE#240. Through their combination in the PreREISE testing branch(EDIT: #240 has been merged). Using the branch for #236, we can:daniel/hifld_top_level_rebased
, and with the branch for this PRChecks should all pass, returning
None
without raising any exceptions.Other entries within
powersimdata.network.hifld.constants
still need to be populated, but those will be just straightforward data-filling and shouldn't affect code functionality, besides enabling the relevant information to be looked up for hifld Grid object by other functions/methods.Time estimate
15-30 minutes. Most new code isn't actually new, just copied with slight tweaks.