Check for extraneous or mis-categorized input parameters

[elenya-grant]

Check for extraneous or mis-categorized input parameters

HUGE THANKS TO @RHammond2 FOR HELPING WITH THIS PR SO MUCH!

This PR introduces a route to resolving Issue #251. In this PR - the user-provided technology configuration is compared against the config attributes of the models that make up that technology after the open-mdao problem has been setup. This is only checked for technologies that have a defined control strategy or dispatch rule set (more than just a cost model and/or a performance model). This is because these are the only cases where models should be using a strict=False when creating the config attribute (currently, this basically only applies to storage cost, performance, and control models).

This will throw an error if the user-provided technology configuration has:

• extra inputs that are not used by any of the models
• inputs that are put in "shared_parameters" but are only used by a single model (i.e., a performance parameter is put under shared parameters)
• something is a shared parameter but is not under shared_parameters

The general methodology/idea is to:

1. Check if a technology has a defined control strategy or dispatch rule set, if so, continue to step 2
2. Get the config attribute of each model for that technology
3. Find the parameters that are shared across all the configs from Step 2 and build the shared_parameters section
4. Check the dictionary built from Step 2 and 3 against the user-provided dictionary. Throw errors if user provided any extraneous inputs or put parameter(s) under the "wrong" parameter section

A follow-on PR will expand the functionality of the check_inputs method to be applied for all technologies (not just ones with a dispatch_rule_set or control_strategy). This would likely requiring adding in special handling for finance models, combined cost and finance models, and require updating more input files (which is why it'd be a separate PR).

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:

• Update inline comments/doc string based on any reviewer feedback to make methodology more clear
• 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 3: Related Issues

This is intended to resolve, or at least mostly resolve Issue #251. Likely will need a follow-on PR for Issue #251 to be fully resolved.

Section 4: Impacted Areas of the Software

Section 4.1: New Files

N/A

Section 4.2: Modified Files

• h2integrate/core/dict_utils.py
  • check_inputs(): new method that checks for extra or mis-categorized inputs
• h2integrate/core/h2integrate_model.py:
  • setup(): added call to check_inputs function
  • create_technology_models(): had to update subgroup naming in the handling of combined performance/cost model creation. This was most visible for the usage of the HOPPComponent as the performance & cost model and having a dispatch rule set. The HOPPComponent was being named as the tech name (hopp), not the model name (HOPPComponent), which was causing issues in the check_inputs() method.
• h2integrate/core/test/test_utilities.py
  • test_check_inputs(): tests that the check_inputs method works as expected
  • create_om_problem(): new method that is used in test_check_inputs
• h2integrate/core/test/inputs/no_duplicates.yaml: added demand_profile under the battery performance parameters so that the battery model can be instantiated properly

Examples that need fixed tech configs:

• 01_onshore_steel_mn: updated combiner, battery, h2_storage
• 02_texas_ammonia: updated combiner, battery, h2_storage
• 12_ammonia_synloop: updated combiner, battery, h2_storage
• 18_pyomo_heuristic_dispatch: updated battery
• 30_pyomo_optimized_dispatch: updated battery
• 09_co2/direct_ocean_capture: updated hopp, combiner, and battery
• 09_co2/ocean_alkalinity_enhancement: updated battery

The fact that these tech config files needed to be updated is proof that the added functionality works as intended (yay!).

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

[genevievestarke]

This is fantastic functionality, @elenya-grant !! I love the tests!

[genevievestarke]

Can we update this doc string?

[RHammond2]

@elenya-grant, based on our previous conversation, I cleaned up the logic so everything is much flatter and with simplified control flow and implementing the additional checks from your notes. All of your core logic was sound, and so I just added a single case to test_check_inputs to do further checking to ensure one of my additions is also checked.

I think the one piece that's missing for resolving #251 is to update the default strict setting to False, and update anywhere from_dict is called, then update the models so that strict is only used when specifically needed.

[elenya-grant]

@RHammond2 - Thank you for cleaning it up and adding another test! I appreciate it! Go team!

In response to this:

I think the one piece that's missing for resolving #251 is to update the default strict setting to False, and update anywhere from_dict is called, then update the models so that strict is only used when specifically needed.

Personally, I still like the usage of strict=True in some models. If I'm running H2I with the VSCode debugger, then if I provide an extraneous input parameter to a model that uses strict=True, then it'll take me to the config class for that model with that error, which is nice because then I can look at the config class attributes without having to search around H2I for the model that threw the error.

Granted, if the larger team agrees with updating the strict setting to False and updating the calls to merge_shared_inputs, then I think that could be done in a separate PR before we close Issue #251

[RHammond2]

@elenya-grant, that makes sense to me, and IIRC we were aiming to make strict=True more of an intentional choice, rather than the default since this PR adds an additional input checking step. Either way, having a second PR to do that plus updating merge_shared_inputs would be ideal to try to keep individual PRs smaller.

[johnjasa]

I love this PR and I'm so glad this is getting addressed! The collaborative nature that you achieved this is just fantastic. @elenya-grant, great movement on the ideas and implementation, and @RHammond2, thanks for streamlining the code so well.

I made a small change to include the tech_config_fpath to the method so the error message can include the file that a user would need to change. In working with some new users recently, I found that our error messages could generally point towards the exact files and sections they'd need to modify, so I hope this change is helpful to those folks.

Great stuff!

[kbrunik]

Converter files where strict=False:

• hopp_wrapper.py
• pem_electrolyzer.py
• iron_dri_base.py
• iron_transport.py
• solar_pysam.py
• steel_eaf_base.py

Not sure if these should be updated to strict=True since in the PR body you mentioned it should only be set to False for technologies that have a defined control strategy or dispatch rule set.

[RHammond2]

I made a small change to include the tech_config_fpath to the method so the error message can include the file that a user would need to change. In working with some new users recently, I found that our error messages could generally point towards the exact files and sections they'd need to modify, so I hope this change is helpful to those folks.

Thanks so much for adding the additional error handling, @johnjasa, I completely missed that aspect when thinking through the streamlining!

[kbrunik]

Looks great to me! Thank you for updating the other models that were unnecessarily using strict = False.

One note for other folks who might look at this, strict = False is necessary for the ECOElectrolyzerPerformanceModelConfig because it's inherited and fully reinstantiated by WOMBATModelConfig and will throw an error because WOMBATModelConfig requires extra inputs but appear extraneous to the ECOElectrolyzerPerformanceModelConfig.

It seems like there's some weird stuff going on with the CI, @RHammond2, it looks like it's the windows tests. Any ideas?