Baseclass for Storage Performance Models

[elenya-grant]

Baseclass for Storage Performance Models

This PR introduces a storage performance baseclass. The baseclass will help standardize the inputs and outputs of storage performance models.

The storage baseclass is in h2integrate/storage/storage_baseclass.py. This would be inherited by the following storage performance models:

• StoragePerformanceModel in h2integrate/storage/storage_performance_model.py
• PySAMBatteryPerformanceModel in h2integrate/storage/battery/pysam_battery.py
• StorageAutoSizingModel in h2integrate/storage/simple_storage_auto_sizing.py

Some notes on the current performance baseclass:

• the setup() method includes logic that would make it easy-to-use for all storage performance models, that is why it checks the configuration class for certain keys.
• the run_storage() method is the main method that should be called at the end of compute(). This takes in the necessary information to simulate the storage performance and set the outputs
• the simulate() method currently exists in the baseclass. This method would be over-written in the PySAMBatteryPerformanceModel

This PR is ready for a full-review.

Section 1: Type of Contribution

• Feature Enhancement
  • Framework
  • New Model
  • Updated Model
  • Tools/Utilities
  • Other (please describe):
• Bug Fix
• Documentation Update
• CI Changes
• Other (please describe):

Section 2: Draft PR Checklist

• Open draft PR
• Describe the feature that will be added
• Fill out TODO list steps
• Describe requested feedback from reviewers on draft PR
• [-] Complete Section 7: New Model Checklist (if applicable)

TODO (once reviewers provided initial feedback):

• Update storage performance models and configuration classes to inherit the baseclass and base configuration class
• Remove commented out code from the baseclass file

Type of Reviewer Feedback Requested (on Draft PR)

Structural feedback:

Implementation feedback:

• I like the setup() method as-is, but I was curious what folks think about inputs that are added based on what's in the config? (Like charge rate and storage capacity only being inputs if those corresponding keys are in the config)
• Options for the simulate() method (which is preferred?)
  1. simulate() lives in the baseclass and is over-written by the PySAM battery model (as it is currently) OR
  2. simulate() raises a not-implemented error in the baseclass and each model has their own version of it (meaning that the method would be the same in the StoragePerformanceModel and StorageAutoSizingModel

Other feedback:

Section 3: General PR Checklist

• PR description thoroughly describes the new feature, bug fix, etc.
• [-] Added tests for new functionality or bug fixes
• Tests pass (If not, and this is expected, please elaborate in the Section 6: Test Results)
• Documentation
  • Docstrings are up-to-date
  • Related docs/ files are up-to-date, or added when necessary
  • Documentation has been rebuilt successfully
  • [-] Examples have been updated (if applicable)
• CHANGELOG.md
  • At least one complete sentence has been provided to describe the changes made in this PR
  • After the above, a hyperlink has been provided to the PR using the following format:
    "A complete thought. [PR XYZ]((https://github.com/NatLabRockies/H2Integrate/pull/XYZ)", where
    XYZ should be replaced with the actual number.

Section 3: Related Issues

Part of Issue #564

Section 4: Impacted Areas of the Software

Section 4.1: New Files

• h2integrate/storage/storage_baseclass.py
  • StoragePerformanceBaseConfig: Base configuration class for storage performance models, currently includes the attributes that are shared across all storage performance models.
  • StoragePerformanceBase: new baseclass for storage performance models
    • setup(): initializes inputs and outputs of storage performance models
    • run_storage(): method that runs the simulate() method and calculates the outputs of the storage performance model
    • simulate(): simple storage simulate method that's used by StoragePerformanceModel and StorageAutoSizingModel (the PySAM battery model overwrites this method)

Section 4.2: Modified Files

• h2integrate/storage/battery/pysam_battery.py: updated to use base config and baseclass
  • with the inheritance of the new baseclass, some output naming changed. Specifically:
    • battery_electricity_discharge -> storage_electricity_discharge
    • battery_electricity_charge -> storage_electricity_charge
    • battery_electricity_out -> storage_electricity_out
• h2integrate/storage/simple_storage_auto_sizing.py: updated to use base config and baseclass
• h2integrate/storage/storage_performance_model.py: updated to use base config and baseclass

Section 4.3: Removed Files

• h2integrate/storage/battery/battery_baseclass.py

Section 5: Additional Supporting Information

Section 6: Test Results, if applicable

[kbrunik]

switching it up from the 3600 I see

[kbrunik]

I like the if statements in the setup()

[kbrunik]

Are we worried about this TODO now that it's coming into a base class?

[elenya-grant]

I'm not sure. I can't think of a better way to do it. Any changes to this logic may also require updating the logic in the setup() method of PyomoControllerBaseClass. I personally don't know of a better way to handle this logic at the moment

[kbrunik]

@johnjasa or @RHammond2 any thoughts?

[jaredthomas68]

Perhaps pulling a specific entry from the split() output would be better. However, I think this should generally work as long as no two technologies have the same name, even if they are the same type (e.g. two batteries in the system should be battery_1 and battery_2 or something similar.

[RHammond2]

I feel like @johnjasa's framing of "should be ok" spells trouble down the road. I tend to avoid this kind of maneuver unless there's a validation involved. In a separate project, I entirely removed a large series of these combined naming structures as it created some extreme brittleness in naming conventions. The other concern is it becomes less clear over time what information is actually being captured, leading to difficulty in maintainability.

[elenya-grant]

If someone has a suggestion on an alternative approach then I'm happy to try it.

[kbrunik]

Potentially we could give each tech it's own unique name, which I think could be helpful on multiple fronts not just dispatch.

self.tech_name = self.options.get("tech_name")

Then:

using_feedback_control = False

connections = self.options["plant_config"].get("tech_to_dispatch_connections", [])

for _source_tech, intended_dispatch_tech in connections:
    if intended_dispatch_tech == self.tech_name:
        self.add_discrete_input("pyomo_dispatch_solver", val=dummy_function)
        using_feedback_control = True
        break

I think this would be a bit more work than this PR is meant to do and relates very much so to #620. I think we could leave this for another PR.

[johnjasa]

Sounds great, let's tackle this in the future as part of #620.

[elenya-grant]

Just to clarify, I do think that we could do battery_1 and battery_2. For example, if we're using a battery and named it battery_1, then the pathname attribute when running H2I is plant.battery.PySAMBatteryPerformanceModel, so the self.tech_group_name is ["plant", "battery_1", "PySAMBatteryPerformanceModel"]. And tech_to_dispatch_connections would be something like:

tech_to_dispatch_connections:
  - [wind, battery_1]
  - [battery_1, battery_1]

so - as its looping through the tech_to_dispatch_connections, the statement if any(intended_dispatch_tech in name for name in self.tech_group_name) should evaluate to True because battery_1 is in the list. I think this would be brittle if the "in" was checking the "in" of a string, but since its an "in" a list - I think its actually sound logic.

[kbrunik]

I like the simulate method in the base class because then it makes it clear that it's a storage method. And I don't mind that it's overwritten in the PySAM battery model

[jaredthomas68]

Ditto, I'm glad to see this in the base class

[kbrunik]

When this is ready I would love to see a more robust docstring explaining how to use run_storage() in classes that inherit this base class

[elenya-grant]

I just pushed up a doctoring to for the run_storage() method, but I will also add comments in the compute() method once we update the other storage performance models to inherit it.

[kbrunik]

Thank you for including an example in the docstring! Very helpful.

[jaredthomas68]

I generally like where this is going. I have a lot of thoughts about the inputs and outputs, some of which I have shared in #521 and will likely need to be addressed in later PRs. Good job bringing more order and clarity to the dispatch/control code.

[jaredthomas68]

Perhaps pulling a specific entry from the split() output would be better. However, I think this should generally work as long as no two technologies have the same name, even if they are the same type (e.g. two batteries in the system should be battery_1 and battery_2 or something similar.

[jaredthomas68]

Ditto, I'm glad to see this in the base class

[kbrunik]

Thanks @elenya-grant for putting this together! It really helps clarify what's unique to each storage method and the base class is very clearly laid out with nice docstrings 😎

I had a couple of small comments regarding the docstrings but should be pretty easy to resolve and won't require another re-review. Thanks again!

[kbrunik]

Love how much simpler the pysam_battery is now!!

[kbrunik]

Potentially we could give each tech it's own unique name, which I think could be helpful on multiple fronts not just dispatch.

self.tech_name = self.options.get("tech_name")

Then:

using_feedback_control = False

connections = self.options["plant_config"].get("tech_to_dispatch_connections", [])

for _source_tech, intended_dispatch_tech in connections:
    if intended_dispatch_tech == self.tech_name:
        self.add_discrete_input("pyomo_dispatch_solver", val=dummy_function)
        using_feedback_control = True
        break

I think this would be a bit more work than this PR is meant to do and relates very much so to #620. I think we could leave this for another PR.

[kbrunik]

Thank you for including an example in the docstring! Very helpful.

[johnjasa]

Thank you for this, Elenya! It makes so many of the storage models much cleaner and easier to understand.

I have some very minor questions/suggestions and also pushed up some small changes. We're on a call right now so we already chatted about my big picture thoughts.

[johnjasa]

Why not have this live in model_baseclasses.py? We have performance and cost baseclasses there already. Should the storage one live here instead of with those others?

[elenya-grant]

I think that sense this is specific to storage performance models it makes sense to live in the storage folder. A model in converters should not inherit this. I think baseclasses that can be used for storage or converters can live in model_baseclasses.py but I think that baseclasses that should only be inherited by a specific type of model should live in that folder. Just like how it wouldn't make sense (to me) for h2integrate/control/control_strategies/pyomo_controller_baseclass.py to live in model_baseclasses - because its specific to control strategies.

[johnjasa]

Thanks, I agree with your logic!

[johnjasa]

This if statement and the comment don't seem to match up. Could you clarify in the code, either the logic or the comment?

[elenya-grant]

@johnjasa did you already address this? it says its outdated for me

[johnjasa]

I haven't modified the part that I have a question about:

        if not getattr(self.config, "charge_equals_discharge", True):
            # add if max_discharge_rate in config
            self.add_input(
                "max_discharge_rate",
                val=self.config.max_discharge_rate,
                units=commodity_rate_units,
                desc="Storage discharge rate",
            )

It's checking for charge_equals_discharge but then the comment mentions max_discharge_rate; it seems unrelated? I might be misinterpreting

[elenya-grant]

They're super related. If charge_equals_dicharge is True - then the discharge rate is the same as the charge rate so we don't need the input for max_discharge rate. If charge_equals_discharge is False, then the discharge rate has to be specified in the config and we set it as its own input. I updated the inline comment to be a bit more clear hopefully.

[johnjasa]

Sounds great, let's tackle this in the future as part of #620.