Skip to content

Conversation

aspeake
Copy link
Collaborator

@aspeake aspeake commented Oct 8, 2025

New ecm_prep.py argument, ecm_field_updates that can generically update any number of ECM fields. Details of the PR include:

  • Fields are updated for every single ECM marked to be prepared or part of a package
  • Any key-value pair can be provided, which will either overwrite the existing ECM field or write a new field
  • Only top level fields are supported (no nested fields)
  • Removed all custom ECM definitions used in the integration tests, and instead just update the climate_zone fields.
  • A new unit test was added and docs updated

@aspeake aspeake requested a review from jtlangevin October 9, 2025 14:51
@jtlangevin
Copy link
Collaborator

  • Only top level fields are supported (no nested fields)

Is it not possible to provide a dict as the val in the key:val pair for the argument? Suggest we explicitly note in the explanation that goes into the docs if users should not provide nested values.

@jtlangevin
Copy link
Collaborator

jtlangevin commented Oct 13, 2025

  • Removed all custom ECM definitions used in the integration tests, and instead just update the climate_zone fields.

I'm not sure if this is the best idea, because ultimately the set of ECMs we include under ./ecm_definitions will be broader than what I included for testing, and we may periodically need to add/subtract from those ECMs. Wouldn't it be better to have a dedicated set for testing that is unaffected by any changes we make to the "core" set in ./ecm_definitions?

Another option that would engage this new capability is to leave the ECM folder ./ecm_definitions/integration_testing in place but change it such that the ECMs there are the same as the versions in ./ecm_definitions (e.g., the ECMs that are in the integration testing folder now, but with climate_zone: "all")and leave the ecm_field_updates for the testing YAML as it is now.

Update fields across all ECMs
*****************************

``--ecm_field_updates`` updates fields of all ECMs selected for preparation or included in packges to be prepared.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check spelling ("packges")

"(C) Bst. HP FS (RTU, NG Ht.) & Env.+"
]
ecm_field_updates:
climate_zone: ["CA", "CO", "MN", "MI", "NY", "MA", "TN", "FL", "TX"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can there be any number of these listed line-by-line? Would be helpful to clarify in the description of this attribute.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, will update

@aspeake
Copy link
Collaborator Author

aspeake commented Oct 14, 2025

  • Only top level fields are supported (no nested fields)

Is it not possible to provide a dict as the val in the key:val pair for the argument? Suggest we explicitly note in the explanation that goes into the docs if users should not provide nested values.

Yes, you could provide a dict to cover nested fields, just can't nest keys in the yml.

@aspeake
Copy link
Collaborator Author

aspeake commented Oct 14, 2025

  • Removed all custom ECM definitions used in the integration tests, and instead just update the climate_zone fields.

I'm not sure if this is the best idea, because ultimately the set of ECMs we include under ./ecm_definitions will be broader than what I included for testing, and we may periodically need to add/subtract from those ECMs. Wouldn't it be better to have a dedicated set for testing that is unaffected by any changes we make to the "core" set in ./ecm_definitions?

Another option that would engage this new capability is to leave the ECM folder ./ecm_definitions/integration_testing in place but change it such that the ECMs there are the same as the versions in ./ecm_definitions (e.g., the ECMs that are in the integration testing folder now, but with climate_zone: "all")and leave the ecm_field_updates for the testing YAML as it is now.

I think the subset of ECMs for testing is covered by the ecm_files argument, right? I am thinking that one of the reasons for this feature would be to avoid having repeated ECM sets. With this we 1) don't need to make updates to two sets so the integration test set won't get stale and 2) we are continuously testing that ecm_definitions is valid

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants