Removed is_electricity_producer#785
Closed
johnjasa wants to merge 5 commits into
Closed
Conversation
Collaborator
Author
|
Waiting on #786 to pass and come in, then I'll revamp this |
Collaborator
Author
|
Closing in favor of #786 once I update it a bit |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove
is_electricity_producerand make finance components commodity-agnosticPreviously, the finance components (
ProFastBase/ProFastLCO/ProFastNPVandNumpyFinancialNPV) inspectedcommodity_typeto decide whether to use electricity units (kW,kWh,USD/(kW*h)) or mass units (kg/h,kg,USD/kg). A helper namedis_electricity_producerwas also used inH2IntegrateModel.create_finance_modelto special-case detection of the commodity-stream technology when the user did not specify one for an electricity finance subgroup.This PR removes that electricity-specific logic from the finance layer so the finance components only know about units, not commodities, and the model-build layer is responsible for selecting the correct units when constructing finance components.
Finance components
ProFastBaseandNumpyFinancialNPVnow expose three explicit unit options:H2IntegrateModel.create_finance_modelis_electricity_producerif commodity == "electricity"branch is gonedefault_commodity_unitsmapping nearfin_compcreation supplies electricity-specific defaults (kW/kWh/USD/(kW*h)) and falls back to mass defaults for any other commodityH2IntegrateModel.create_technology_modelsself.tech_perf_models: dict[str, om.System], populated as each tech group is built, so other model-build steps can look up a tech's performance-model component directly bytech_namerather than having to walk the OpenMDAO subsystem tree.commodity_stream_definitions.pyis_electricity_producerfunction and its module-docstring item.test_is_electricity_producer.Section 1: Type of Contribution
Section 2: Draft PR Checklist
Section 3: General PR Checklist
docs/files are up-to-date, or added when necessaryCHANGELOG.md"A complete thought. [PR XYZ]((https://github.com/NatLabRockies/H2Integrate/pull/XYZ)", where
XYZshould be replaced with the actual number.Section 4: Related Issues
Resolves #604
Section 5: Impacted Areas of the Software
Section 5.1: New Files
Section 5.2: Modified Files
A lot!
Section 6: Additional Supporting Information
This change is behavior-preserving for users who build models through
H2IntegrateModelwith the standard YAML configuration: electricity finance subgroups still default tokW/kWh/USD/(kW*h), mass-commodity subgroups still default tokg/h/kg/USD/kg, and default commodity-stream detection still finds the same producer technologies (now identified by the model's owncommodityattribute rather than a hard-coded prefix table).Section 7: Test Results, if applicable
h2integrate/core/test/andh2integrate/finances/test/: 67 passed locally.