Skip to content

Reworked how financial model units and commodity streams are defined#786

Open
elenya-grant wants to merge 17 commits into
NatLabRockies:developfrom
elenya-grant:finance_commodity_units
Open

Reworked how financial model units and commodity streams are defined#786
elenya-grant wants to merge 17 commits into
NatLabRockies:developfrom
elenya-grant:finance_commodity_units

Conversation

@elenya-grant

@elenya-grant elenya-grant commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Reworked how financial model units and commodity streams are defined

Updated finance models to not have hard-coded logic related to defining units by taking advantage of the compute_units and copy_units options when defining inputs and outputs in OpenMDAO.

The main contribution of this PR is the addition of the _compute_price_units() function.

This PR is ready for a high-level review. The file changes are pretty minor so I'm hoping the changes are pretty easy to follow.

We also removed is_electricity_producer as part of this!

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)

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 4: Related Issues

Section 5: Impacted Areas of the Software

Section 5.1: New Files

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

Section 5.2: Modified Files

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

Section 6: Additional Supporting Information

Section 7: Test Results, if applicable

Section 8 (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
    • Run generate_class_hierarchy.py to update the class hierarchy diagram in docs/developer_guide/class_structure.md

@johnjasa johnjasa mentioned this pull request Jun 18, 2026
26 tasks
@elenya-grant elenya-grant added ready for review This PR is ready for input from folks and removed in progress labels Jun 19, 2026
@johnjasa johnjasa marked this pull request as ready for review June 22, 2026 19:00
@johnjasa johnjasa self-requested a review June 22, 2026 22:18
@elenya-grant elenya-grant requested a review from kbrunik June 23, 2026 16:12
Comment thread h2integrate/core/h2integrate_model.py Outdated
Comment on lines +1344 to +1353
# Default logic for tech-names and the primary commodity streams
default_techs_to_commodities = {
"electrolyzer": "hydrogen",
"geoh2": "hydrogen",
"ammonia": "ammonia",
"doc": "co2",
"oae": "co2",
"methanol": "methanol",
"air_separator": "nitrogen",
}

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.

Do we still need this? I think it might not be strictly necessary. I might've made this 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.

Okay, by saying that commodity_stream is req'd now, we can get rid of this -- so I did!

@jaredthomas68 jaredthomas68 left a comment

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 didn't have time to give a detailed review, but I skimmed through the changes and I like this. Nice code simplification and at the same time reducing some obfuscation on commodity streams/sources.

@johnjasa johnjasa changed the title Units in finance models Reworked how financial model units and commodity streams are defined Jun 25, 2026
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.

3 participants