Skip to content

Remove system-level outputs from storage and replace with demand component (follow-on to 631)#666

Open
elenya-grant wants to merge 62 commits intoNatLabRockies:developfrom
elenya-grant:storage/standalone_outputs
Open

Remove system-level outputs from storage and replace with demand component (follow-on to 631)#666
elenya-grant wants to merge 62 commits intoNatLabRockies:developfrom
elenya-grant:storage/standalone_outputs

Conversation

@elenya-grant
Copy link
Copy Markdown
Collaborator

@elenya-grant elenya-grant commented Apr 10, 2026

Remove system-level outputs from storage and replace with demand component (follow-on to 631)

There has been a blurry line distinguishing system-level calculations and calculations done in storage performance models. This PR builds on PR #631 to resolve Issue #521. The storage performance outputs that depend on demand have been removed and the usage of demand components in the examples is more common. The outputs removed from storage performance models are:

  • unmet_{commodity}_demand_out: demand that has not been met
  • unused_{commodity}_out: production that exceeds the demand
  • storage_{commodity}_out: charge/discharge profile

The outputs with different calculations in the storage performance models are:

  • {commodity}_out: this used to be the combined commodity out from storage and whatever was input to storage, minus any production that exceeds the demand. This is now what storage_{commodity}_out used to be
  • capacity_factor: used to be the based on the combined commodity out, now it based on the sum of the charge/discharge profile
demand_calcs demand_commodity_out

This PR helps to better distinguish the differences between individual storage performance models and "system-level" performance calculations. This PR also enables the ability for the storage demand to be distinct from the overall demand. For example, if we wanted to use a battery to try to just keep an electrolyzer system on (at 10% power), then the demand profile to the battery would be 10% of the electrolyzer capacity, whereas the demand for the demand component would be equal to the electrolyzer capacity.

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:

  • Get PR Update Naming Convention in Open-Loop Converter Control Strategies #631 merged in
  • Update storage_baseclass.py so that demand is only an input if using feedback control (not doing just yet)
  • make figures to show what's happening in demand components for different use-cases
    • these figures have been added to docs/demand/demand_components.md
  • add test to test_all_examples for example 13
  • update or add doc page for example 13 in controller docs

Type of Reviewer Feedback Requested (on Draft PR)

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

This would resolve Issue #521

Section 4: Impacted Areas of the Software

Section 4.1: New Files

  • examples/13_dispatch_for_electrolyzer/: new example that highlights setting the storage demand as a different value than the demand component.
  • resource_files/wind/30.6617_-101.7096_2013_wtk_v2_60min_utc_tz.csv: wind resource file for example 23
  • docs/demand/demand_demo.md: new doc page for Example 13 that highlights use of demand components.
  • docs/technology_models/storage_models_index.md: new doc page describing the inputs and output calculations of the storage performance models.

Section 4.2: Modified Files

  • h2integrate/core/h2integrate_model.py
    • create_finance_model(): Updated logic so that any model without a cost model does not have to be included in the technologies list for a finance group. Was previously hard-coded so that only combiners didn't have to be included in the technologies list.
  • h2integrate/storage/storage_baseclass.py: modified StoragePerformanceBase
    • setup(): removed outputs related to demand
    • run_storage(): removed calculations of outputs related to demand. Updated calculation of capacity factor.
  • examples/test/test_all_examples.py::test_ng_demand_example: new test for Example 23
  • examples/test/test_all_examples.py::::test_electrolyzer_demand: new test for Example 13

Examples that were updated

  • Examples that had added combiners to combine storage outputs with the input to storage: 01, 02, 03/co2_hydrogenation_doc, 09/doc, 09/oae, 30
  • Examples that had added combiners and load demand components: 12, 16, 24, 30
  • Examples that had storage models as commodity streams (required additional combiner(s) and a new load demand component): 14, 18, 19, 29

Tests that were updated

Section 5: Additional Supporting Information

Section 6: Test Results, if applicable

  • h2integrate/core/test/test_framework.py::test_system_order
  • h2integrate/core/test/test_framework.py::test_technology_connections

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 and others added 27 commits March 26, 2026 15:17
@bayc
Copy link
Copy Markdown
Collaborator

bayc commented Apr 13, 2026

@elenya-grant Trying to look through this, and wondering if you can put together a diagram or figure showing the inputs/outputs of the current system and proposed solution? Would help a lot to see what the differences are.

Thanks for adding the plots! That definitely helps me understand what the code is doing, which is moving the difference calculations out of the storage model. I think these outputs were originally developed with converters in mind as well and trying to apply the same set of inputs/outputs across technologies to be able to easily know how much of a commodity was consumed and how much, if any, demand is left unmet after sequentially working through each component.

My understanding now is that the difference calculations are being moved to demand components, and agree with @kbrunik that it would be helpful to see this built out in an example, so supportive of that! This would help me understand any other impacts to the dispatch/control flow and demand passing through converters as well.

@elenya-grant elenya-grant requested a review from kbrunik April 13, 2026 22:02
Copy link
Copy Markdown
Collaborator

@genevievestarke genevievestarke left a comment

Choose a reason for hiding this comment

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

Overall, I think this looks really good! Great progress on this, @elenya-grant ! I have a few comments, and I'd like to look at the final docs pages when they're ready, as well :)


electrolyzer_capacity_MW = 60
h2i.prob.set_val("battery.electricity_demand", 0.1 * electrolyzer_capacity_MW, units="MW")
h2i.prob.set_val("elec_load_demand.electricity_demand", electrolyzer_capacity_MW, units="MW")
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.

Ooooh, nice, I really like this example!

# Performance model outputs
outputs[f"rated_{self.commodity}_production"] = discharge_rate
outputs[f"total_{self.commodity}_produced"] = np.sum(total_commodity_out)
outputs[f"total_{self.commodity}_produced"] = np.sum(storage_commodity_out)
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 this should be based just on the sum of storage discharge (otherwise you'll get a very small number).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

for sure! I hear you - the benefit of this method is that, suppose we have something a system that has technology connections that look like this:

technology_interconnections:
  # combine the generation and input it to the battery
  - [wind, gen_combiner, electricity, cable]
  - [pv, gen_combiner, electricity, cable]
  - [gen_combiner, battery, electricity, cable]
  # combine the battery output with the wind and solar generation
  - [battery, elec_combiner, electricity, cable]
  - [gen_combiner, elec_combiner, electricity, cable]

Then, it's nice because it follows the same logic as the converters and the following two things are equal:

tot_electricity_out = model.prob.get_val("battery.total_electricity_produced") + model.prob.get_val("wind.total_electricity_produced") + model.prob.get_val("wind.total_electricity_produced")

assert tot_electricity_out == model.prob.get_val("elec_combiner.electricity_out").sum()

I'd be interested to hear what other folks think.

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 this gets to Jared's comment in #521 to have "outputs of commodity flows mutually exclusive and collectively exhaustive" and it's nice to have the storage not output values from other components.

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 see your reasoning @elenya-grant and I think it works for your use case. My concern is that the capacity factor is based on the total commodity production, but a storage systems capacity factor is calculated based only on discharge and divided by possible discharged continuously. If we make sure the capacity factor calculation is being handled correctly, then I think I'm ok with the positive and negative included in the sum. We will need to be very clear about its meaning in docs and doc strings. I am still concerned about the misleading name "produced" when the sum includes commodity used.

Copy link
Copy Markdown
Collaborator Author

@elenya-grant elenya-grant Apr 14, 2026

Choose a reason for hiding this comment

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

Okay - so I think the main question is whether aggregated outputs from the storage models (annual_commodity_produced, total_commodity_produced, and capacity_factor) should be based on either 1) charge and discharge profile or 2) just the discharge profile.

My main concern is with the capacity factor, since that's passed to combiners and is a metric used in finance calculations. I don't have strong feelings about annual_commodity_produced and total_commodity_produced, since those are more useful as "summary outputs" and not used in downstream models.

If storage model outputs are based on the charge and discharge profile - then this is similar to how converter outputs are calculated wrt the commodity_out profile (nice). But - if the battery charges more than discharges, then we would have negative values (could cause problems). If the outputs are negative though - then I think produced makes sense in the name.

If storage model outputs are based only on the discharge profile - then we'd always have positive outputs, but we couldn't sum these together with the converter technologies like I mentioned above. Aka - it'd require special treatment of converter technologies.

Regardless of what we decide, I will update the doc page for the storage performance models to detail how these calculations are done.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Larger question that I think would help us figure out what direction to go in. How would you calculate the capacity factor of a wind+battery system?

wind_aep = sum(wind_generation)
wind_cf = wind_aep/(wind_capacity*8760)

# Like below?
hybrid_cf = sum(wind_generation)/(8760*(wind_capacity + battery_discharge_rate))

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 it is important to keep standard procedures. I also think we can just take the positive part of the commodity out for all techs and divide by their nameplate capacity and then the capacity factor calculation will be the same for all technologies.

     cf = sum(positive(commodity out)/(rate unit rating * hours in year)

From the NLR ATB:

"Capacity Factor

The cost and performance of the battery systems are based on an assumption of approximately one cycle per day. Therefore, a 4-hour device has an expected capacity factor of 16.7% (4/24 = 0.167), and a 2-hour device has an expected capacity factor of 8.3% (2/24 = 0.083). Degradation is a function of the usage rate of the model, and systems might need to be replaced at some point during the analysis period. We use the capacity factor for a 4-hour device as the default value for ATB because 4-hour durations are anticipated to be more typical in the utility-scale market.

The hybrid capacity factor is an important question. I do want to avoid having our own internal definition of capacity factor, but I also see the utility of @elenya-grant's proposal for using positive and negative in the CF calc

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I added standard_capacity_factor as an output that uses the equation @jaredthomas68 proposed. This is documented in the new storage doc page docs/technology_models/storage_models_index.md. Please let me know your thoughts.

)


def calculate_combined_outputs(storage_charge_discharge, commodity_in, commodity_demand):
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 this makes sense to keep the tests the same, but maybe as a future PR we can think about whether we need to calculate all of these for the controller tests. (Or just test these for the controllers by themselves)

Copy link
Copy Markdown
Collaborator Author

@elenya-grant elenya-grant Apr 13, 2026

Choose a reason for hiding this comment

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

I agree - it was only necessary because most of the subtests were on these variables that are now calculated in demand components - but I agree.

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.

calculate_combined_outputs may be better placed in tools.py or similar instead of copying it around to various scripts. I also hesitate to base so many tests on a function, but it would be rather clunky to store values for all of these.

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 agree with @genevievestarke that we should remove tests on values not calculated by these components any more, but in a future PR.

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.

It could have been my fault in the merge too, sorry if this isn't used any more

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think that maybe we revisit this in a future PR. the options are:

  • update the tests to test different outputs (don't test the unmet demand, excess commodity, etc)
  • keep the tests as-is and move the calculate_combined_outputs to conftest.py?
  • update the tests to use a demand component for the calculations instead of calculate_combined_outptus

Copy link
Copy Markdown
Collaborator Author

@elenya-grant elenya-grant Apr 14, 2026

Choose a reason for hiding this comment

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

Decided we're going to do the first option in a future PR. made Issue #676

Copy link
Copy Markdown
Collaborator

@kbrunik kbrunik left a comment

Choose a reason for hiding this comment

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

Thanks @elenya-grant! I think this PR helps continue to clarify/separate "storage" models from non-storage/more generic things like the controls logic and the demand. I like that none of the actual calculations/test values changed in this PR just mostly reorganizing and adding some sweeeeet docs. I have a few non-blocking comments but I'd love if you'd take a look and respond to them before merging.

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.

Looking at this figure, the red is really hard to see even for a color seeing person. I realize this isn't a new addition in this PR but do we have to code to maybe change to color and that wouldn't be too hard? If the answer is no then don't do it.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I don't remember where the code is that I had to make this figure initially - so - I can try to re-make it but it'd take 30min+

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.

are the added figures color-blind friendly?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure...could you suggest some colors that you'd like instead?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Noted in Issue #667

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 these figures really help illustrate what the calculations are doing :) thank you!

```{code-cell} ipython3
electrolyzer_capacity_MW = 60

# Set the battery demand equal to the minimum electricity needed to keep the electrolyzer on
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.

Wait, didn't you already set this in the tech_config?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes - I may have pushed up changes since your comment (the line numbers are confusing me). Before the part where I call set_val() now - I have this line:

If we wanted to change the demand profiles for the battery (battery) or the demand component (elec_load_demand) to be different than the demand profiles specified in the technology config, we can do that using set_val:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

commented out the python code in that section but left the note so folks understand.

```


Plot the battery performance:
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.

Sick plot!!!!

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.

Was there not an existing wind resource file that worked for this example?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

unfortunately no.. I don't like that I had to add it either (it was for example 23, not the new example I added). I can change the location to a location that we already have weather data files for and remove this - I was just trying to not change example 23 too much.

)


def calculate_combined_outputs(storage_charge_discharge, commodity_in, commodity_demand):
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.

It could have been my fault in the merge too, sorry if this isn't used any more

@kbrunik kbrunik added the ready for review This PR is ready for input from folks label Apr 14, 2026
Copy link
Copy Markdown
Collaborator

@jaredthomas68 jaredthomas68 left a comment

Choose a reason for hiding this comment

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

This is looking pretty good. I think it makes operation more transparent to the users and will allow more flexibility ultimately. I do have a few comments/changes. My biggest concern is that the storage capacity factor be calculated correctly.

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.

This is a very nice demo, well done putting this together!


The following example is an expanded form of `examples/13_dispatch_for_electrolyzer`.

The technology interconnections:
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 the user would benefit from an xdsm diagram here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

heard - how do I make that?

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.

image

We could use the built-in XDSM-maker in H2I, callable via create_xdsm on the H2IntegrateModel object.
Or, we could use pyXDSM manually like I did for this above.
@jaredthomas68, is this in line with what you're thinking? What would you change?

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.

Yes, that is exactly what I was thinking. It would be nice to use the built-in method as part of the example, but all I was asking for is including the figure to visualize the connections.

Copy link
Copy Markdown
Collaborator Author

@elenya-grant elenya-grant Apr 14, 2026

Choose a reason for hiding this comment

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

thanks! I added it in

ax.set_ylim([0, 70])
ax.legend(bbox_to_anchor=(1.0, 0.5), loc="center left", borderaxespad=0, framealpha=0.0)
ax.set_ylabel("Electricity (MW)")
ax.set_xlabel("Time (hours)")
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.

You did a very nice job with the figures in this page. Clean and clear. Non-blocking suggestion here, but I would like to see the storage SOC and charge and discharge plots directly. I think it would make the storage operation a little more clear.

Copy link
Copy Markdown
Collaborator Author

@elenya-grant elenya-grant Apr 14, 2026

Choose a reason for hiding this comment

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

done!

Comment on lines +18 to +29
# combine the h2 from the electrolyzer and the h2_storage
- [electrolyzer, h2_combiner, hydrogen, pipe]
- [h2_storage, h2_combiner, hydrogen, pipe]
# connect the h2 supply to the methanol system
- [h2_combiner, methanol, hydrogen, pipe]
# connect the doc to co2 storage
- [doc, co2_storage, co2, pipe]
- [co2_storage, methanol, co2, pipe]
# combine the co2 from the doc and the co2_storage
- [doc, co2_combiner, co2, pipe]
- [co2_storage, co2_combiner, co2, pipe]
# connect the co2 supply to the methanol system
- [co2_combiner, methanol, co2, pipe]
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 am a little concerned with the amount of connections needed here, but I don't think it can be helped if we are going to move demand outside of storage, which I do see value in.

)


def calculate_combined_outputs(storage_charge_discharge, commodity_in, commodity_demand):
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.

calculate_combined_outputs may be better placed in tools.py or similar instead of copying it around to various scripts. I also hesitate to base so many tests on a function, but it would be rather clunky to store values for all of these.

)


def calculate_combined_outputs(storage_charge_discharge, commodity_in, commodity_demand):
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 agree with @genevievestarke that we should remove tests on values not calculated by these components any more, but in a future PR.

Comment on lines +83 to +87
if isinstance(self.demand_profile, list | np.ndarray):
user_input_dmd = True if sum(self.demand_profile) > 0 else False
else:
user_input_dmd = True if self.demand_profile > 0 else False

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.

np.sum works on scalars and array-like. Also, the comment was confusing to me. Also, what if
user defines a negative profile?

Suggested change
if isinstance(self.demand_profile, list | np.ndarray):
user_input_dmd = True if sum(self.demand_profile) > 0 else False
else:
user_input_dmd = True if self.demand_profile > 0 else False
# Check for user-define demand profile
user_input_dmd = True if np.sum(self.demand_profile) > 0 else False

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Thanks on the suggestion - I will push up that change shortly (also with an updated comment).

The question of a negative demand profile is interesting - that is something that we don't have a way of handling for any converter model that can take demand as an input or in the demand components. I think that we should discuss how we'd want to handle that and make a separate PR with whatever route seems best.

# Performance model outputs
outputs[f"rated_{self.commodity}_production"] = discharge_rate
outputs[f"total_{self.commodity}_produced"] = np.sum(total_commodity_out)
outputs[f"total_{self.commodity}_produced"] = np.sum(storage_commodity_out)
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 see your reasoning @elenya-grant and I think it works for your use case. My concern is that the capacity factor is based on the total commodity production, but a storage systems capacity factor is calculated based only on discharge and divided by possible discharged continuously. If we make sure the capacity factor calculation is being handled correctly, then I think I'm ok with the positive and negative included in the sum. We will need to be very clear about its meaning in docs and doc strings. I am still concerned about the misleading name "produced" when the sum includes commodity used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review This PR is ready for input from folks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants