Peak load management heuristic control

[jaredthomas68]

Peak load management heuristic control

This PR adds Peak load management heuristic control to H2I. This does not do demand dispatch, but rather dispatches based on peaks in the provided load and rules defined by the user.

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:

• Step 1
• Step 2

Type of Reviewer Feedback Requested (on Draft PR)

I am primarily looking for high-level structural and implementation feedback at this point
Structural feedback:

Implementation feedback:

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

Section 4: Impacted Areas of the Software

Section 4.1: New Files

• path/to/file.extension
  • method1: What and why something was changed in one sentence or less.

Section 4.2: Modified Files

• path/to/file.extension
  • method1: What and why something was changed in one sentence or less.

Section 5: Additional Supporting Information

Section 6: Test Results, if applicable

Section 7 (Optional): New Model Checklist

• Model Structure:
  • Follows established naming conventions outlined in docs/developer_guide/coding_guidelines.md
  • Used attrs class to define the Config to load in attributes for the model
    • If applicable: inherit from BaseConfig or CostModelBaseConfig
  • Added: initialize() method, setup() method, compute() method
    • If applicable: inherit from CostModelBaseClass
• Integration: Model has been properly integrated into H2Integrate
  • Added to supported_models.py
  • If a new commodity_type is added, update create_financial_model in h2integrate_model.py
• Tests: Unit tests have been added for the new model
  • Pytest-style unit tests
  • Unit tests are in a "test" folder within the folder a new model was added to
  • If applicable add integration tests
• Example: If applicable, a working example demonstrating the new model has been created
  • Input file comments
  • Run file comments
  • Example has been tested and runs successfully in test_all_examples.py
• Documentation:
  • Write docstrings using the Google style
  • Model added to the main models list in docs/user_guide/model_overview.md
    • Model documentation page added to the appropriate docs/ section
    • <model_name>.md is added to the _toc.yml

[elenya-grant]

just left some initial comments/questions - haven't done a deep dive yet (so some of my questions/comments may be silly or I'll be able to answer during a deep-dive) but plan to do a deeper review by Thursday morning. I only looked at the changes and additions to the control classes but will review the tests in the second review I do.

Overall looks like a great start - most of my comments were small or were questions!

[elenya-grant]

should this be inputs[f"{self.config.commodity}_demand"] instead of the demand from the config?

[jaredthomas68]

Some of the reasoning for this is in my comment here: #641 (comment). I guess I can split up demand and time stamp as separate inputs so we can use the input like the other controllers.

[jaredthomas68]

I have split up demand and date_time

[elenya-grant]

Howdy! I gave this more of a deeper look! I think I'm a little confused on how this method works (I didn't try to understand it super hard yet) - so most of my comments were nitpicks or questions. My only blocking comment is about the error being removed from load_plant_yaml - I don't think that error message should be removed at this time.

I think a visual (or two) may be nice to explain some of the inputs to the controller - I think if a doc page with some visuals and explanation on the inputs would be super helpful in making it easier for users to understand how to change the control input parameters based on their use-case.

[elenya-grant]

the start-time format in the plant config is defined as being: mm/dd/yyyy HH:MM:SS or mm/dd HH:MM:SS and defaults to 01/01 00:30:00 (doesn't include a year because it was initially going to be used with resource data and the year may change based on the resource year). The format here does not match - do you think we could make sure that the format is consistent mm/dd/yyyy instead of yyyy-mm-dd?

I made a similar function when I was starting on the resource models (it never made it in) but it handles whether a year was added or not:

from datetime import datetime, timezone, timedelta

def make_time_profile(
    start_time: str,
    dt: float | int,
    n_timesteps: int,
    time_zone: int | float,
    start_year: int | None = None,
):
    """Generate a time-series profile for a given start time, time step interval, and
    number of timesteps, with a timezone signature.

    Args:
        start_time (str): simulation start time formatted as 'mm/dd/yyyy HH:MM:SS' or
            'mm/dd HH:MM:SS'
        dt (float | int): time step interval in seconds.
        n_timesteps (int): number of timesteps in a simulation.
        time_zone (int | float): timezone offset from UTC in hours.
        start_year (int | None, optional): year to use for start-time. if start-time
            is formatted as 'mm/dd/yyyy HH:MM:SS' then will overwrite original year.
            If None, the year will default to 1900 if start-time is formatted as 'mm/dd HH:MM:SS'.
            Defaults to None.

    Returns:
        list[datetime]: list of datetime objects that represents the time profile
    """

    tz_utc_offset = timedelta(hours=time_zone)
    tz = timezone(offset=tz_utc_offset)
    tz_str = str(tz).replace("UTC", "").replace(":", "")
    if tz_str == "":
        tz_str = "+0000"
    # timezone formatted as ±HHMM[SS[.ffffff]]
    start_time_w_tz = f"{start_time} ({tz_str})"
    if len(start_time.split("/")) == 3:
        if start_year is not None:
            start_time_month_day_year, start_time_time = start_time.split(" ")
            start_time_month_day = "/".join(i for i in start_time_month_day_year.split("/")[:-1])
            start_time_w_tz = f"{start_time_month_day}/{start_year} {start_time_time} ({tz_str})"

        t = datetime.strptime(start_time_w_tz, "%m/%d/%Y %H:%M:%S (%z)")
    elif len(start_time.split("/")) == 2:
        if start_year is not None:
            start_time_month_day, start_time_time = start_time.split(" ")
            start_time_w_tz = f"{start_time_month_day}/{start_year} {start_time_time} ({tz_str})"
            t = datetime.strptime(start_time_w_tz, "%m/%d/%Y %H:%M:%S (%z)")
        else:
            # NOTE: year will default to 1900
            t = datetime.strptime(start_time_w_tz, "%m/%d %H:%M:%S (%z)")
    time_profile = [None] * n_timesteps
    time_step = timedelta(seconds=dt)
    for i in range(n_timesteps):
        time_profile[i] = t
        t += time_step
    return time_profile

[jaredthomas68]

Thanks for the extended function. I personally much prefer the international format "yyyy-mm-dd", but I understand their being an existing approach. I went ahead and changed the function to handle timezone and added a function to make the time series with direct inputs rather than a config. I do not have much use for the second value of a timestamp, but I did adjust to return python datetime format. We can continue to discuss exact desired format, but I would prefer to make the timeseries in a standard date-time format and allow users and developers to adjust to lists of integers or whatever other format they need from there.

[elenya-grant]

if the plant config is loaded using load_plant_yaml(), then the start time should always be included. Aka - I don't think we should have default values in both the modeling schema and this function. But - I'm happy to see a function like this get in!

[jaredthomas68]

Good feedback. I have added a default timezone and removed the default start_time in this function

[elenya-grant]

update description in plant config

[jaredthomas68]

Done

[elenya-grant]

comment for max_capacity is wrong, should say 10 MWh

[jaredthomas68]

Done

[elenya-grant]

could you add in the other attributes to the doc string? Like peak_range, advance_discharge_period, delay_charge_period, allow_charge_in_peak_range, and min_peak_proximity?

[jaredthomas68]

Done

[elenya-grant]

should the dictionary inputs be checked I the __attrs_post_init__ method to check that they have the right keys?

[jaredthomas68]

Done

[elenya-grant]

why is this a staticemethod rather than just a normal method? (same with _normalize_peak_range?)

[jaredthomas68]

These are static methods because they need to be called with different attributes of the class rather than the exact same attributes in order each time. This also means the output does not have a consistent target.

[elenya-grant]

could these inline comments get moved closer to where that logic is represented in the code?

[jaredthomas68]

I moved them to the docstring as I think that makes more sense.

[elenya-grant]

The description of this compute method makes it seem really similar to the DemandOpenLoopStorageController and the HeuristicLoadFollowingControl - could you update the doctoring to explain the peak-shaving novelty of this?

[jaredthomas68]

Done

[vijay092]

Very exciting work @jaredthomas68! Thanks for putting this together in such a short time! I did a full pass through h2integrate/control/control_strategies/storage/plm_openloop_storage_controller.py and left comments wherever I spotted small issues. Feel free to address them as you see fit.

[vijay092]

Is this supposed to be a tuple?

[jaredthomas68]

Nope, thanks for the catch. Fixed

[vijay092]

default = None

[jaredthomas68]

I intentionally left off the default because I want the user to be very aware of how they are using this controller and if they are excluding a supervisory demand profile or not.

[vijay092]

Same problem for advance_discharge_period, right?

[jaredthomas68]

No, advance_discharge_period uses a unit, val paradigm instead of time stamps.

[vijay092]

Never used.

[jaredthomas68]

Thanks. Removed

[vijay092]

I think it is worth adding a length check against self.n_timesteps somewhere.

[jaredthomas68]

I don't think so. The time series builds on the same config that self.n_timesteps comes from, so they should be the same by default.

[jaredthomas68]

I went ahead and added a check just in case. Won't hurt.

[vijay092]

Good to add check for when supervisor is None.

[jaredthomas68]

Supervisor should never be none in this function because the first argument is always treated as the most important peaks. I changed the naming and doc string to make this more clear.

[jaredthomas68]

I also added the check you suggested

[vijay092]

Method name is misleading. It actually computes "allow_charge"?

[jaredthomas68]

Great point. I've changed the name.

[vijay092]

Note for future:
discharging is only set to False when soc <= soc_min. If the battery doesn't fully drain during the event duration, discharging will continue to stay True

[jaredthomas68]

That is by design. The intended operation is to fully charge and then fully discharge, not try to meet a demand, so the battery should fully discharge. If you have suggestions for catching corner cases on this I'm all ears.

[kbrunik]

I think this should be documented in the docpage on the peak load management, I would have expected the battery to stop discharging once the event is over.

[jaredthomas68]

The event period is pretty loose. We find the exact peak, but discharge leading up to and after that peak. I will include more in a docs page.

[vijay092]

Suggest adding charging = False here in case charging hasn't been set to False in the previous timestep.

[jaredthomas68]

Done

[vijay092]

The first and third terms are the same.

[jaredthomas68]

Yes, I was trying to lean into code reuse and hoping I could find a good way, but I went ahead and removed the duplicate.

[genevievestarke]

Really great PR, @jaredthomas68!
I agree with other comments that a docs page would be nice for users, but I know that's usually one of the last things in a PR!

The main blocking comment I have is the setting of the charging and discharging variables. Let me know if you want to discuss!

[genevievestarke]

I believe these first three are included in the base class, so they aren't usually included in the inherited class.

[jaredthomas68]

Removed

[genevievestarke]

Same comment here (already in base class)

[jaredthomas68]

Removed

[genevievestarke]

I don't think the name of the variable here matches the doc string. Is it specifically for a supervising entity?

[jaredthomas68]

Names have been changed and unified

[genevievestarke]

Many of these parameters are the same as those in DemandOpenLoopStorageController. Could this class inherit from that class?

[jaredthomas68]

Possibly, but only in terms of parameters. Thoughts on this @johnjasa ?

[jaredthomas68]

I added these repeat parameters to the base class as optional based on the value of a class flag that child classes need to set to True for the additional parameters to be required.

[genevievestarke]

I agree! Secondary peaks being the only peaks object that is always computed is a bit confusing. I think this point it true throughout this section, where secondary_demand_profile is actually the main profile.

[genevievestarke]

can n_max_events be zero, or should it be None if it's zero?

[jaredthomas68]

n_max_events set to zero would mean the controller does nothing and the battery will never be used for the case when only one load is provided. When two loads are provided then the supervisory load would have no impact if n_max_events is zero.

[genevievestarke]

I think the wording could be "original and override" maybe? Just trying to head off confusion about secondary being default behavior.

[jaredthomas68]

I changed the names in this method to peaks_1 and peaks_2 since they don't always correspond to the outer scope.

[genevievestarke]

How does this normalize the peak range?

[jaredthomas68]

It does not. The normalize was more of get in the normal format for computations. Bad naming. I changed it to _parse_peak_range

[genevievestarke]

Is this repeated code?

[jaredthomas68]

Yes, it is the same as in DemandOpenLoopStorageController. I originally wanted to inherit, but I'm not sure there is sufficient code reuse for that. I'll give it another go.

[jaredthomas68]

I also wanted to limit inheritance depth

[jaredthomas68]

I have moved these checks to StorageOpenLoopControlBase

[genevievestarke]

I think discharging = False could also be added here

[genevievestarke]

Can discharging and charging be False at the same time? How is that set?

[jaredthomas68]

Good question. The not discharging or charging is handled by rest of the logic. For example, if the battery is fully charged and we are not at a peak, then the battery does nothing.

[jaredthomas68]

The flags are more of what action could be taken in order to make sure the battery fully discharges before charging and fully charges before discharging.