adding validation of the number of options in the ApplyUpgrade measure #330
Conversation
| measure_xml_filename = os.path.join( | ||
| cfg["buildstock_directory"], "measures", "ApplyUpgrade", "measure.xml" | ||
| ) |
There was a problem hiding this comment.
I'm using the measure.xml because it exists in the same place in both ResStock and ComStock, and it's more straightforward and less error prone to parse xml from python than ruby code.
| if os.path.exists(measure_xml_filename): | ||
| measure_xml_tree = objectify.parse(measure_xml_filename) | ||
| measure_xml = measure_xml_tree.getroot() | ||
| n_options_in_measure = 0 | ||
| n_costs_per_option_in_measure = 0 | ||
| for argument in measure_xml.arguments.argument: | ||
| m_option = re.match(r"^option_(\d+)$", str(argument.name)) | ||
| if m_option: | ||
| option_number = int(m_option.group(1)) | ||
| n_options_in_measure = max(option_number, n_options_in_measure) | ||
| m_costs = re.match(r"^option_(\d+)_cost_(\d+)_value", str(argument.name)) | ||
| if m_costs: | ||
| cost_number = int(m_costs.group(2)) | ||
| n_costs_per_option_in_measure = max(cost_number, n_costs_per_option_in_measure) |
There was a problem hiding this comment.
Here I'm counting up the number of options and costs per option in the ApplyUpgrade measure.
There was a problem hiding this comment.
Would it speed things up to not check every option's number of costs (since we know it's the same number for every option)?
There was a problem hiding this comment.
This already runs pretty fast enough. It seems like an unnecessary optimization to me. Since we have to loop over all the arguments anyway, it's just as easy and simpler to check on each one.
| n_options_in_cfg = 0 | ||
| n_costs_in_cfg = 0 | ||
| for upgrade in cfg.get("upgrades", []): | ||
| options = upgrade.get("options", []) | ||
| n_options = len(options) | ||
| n_costs = max(len(option.get("costs", [])) for option in options) | ||
| n_options_in_cfg = max(n_options, n_options_in_cfg) | ||
| n_costs_in_cfg = max(n_costs, n_costs_in_cfg) |
There was a problem hiding this comment.
Here I count up the number of options and costs per option required in the project file.
| err_msgs = [] | ||
| if n_options_in_cfg > n_options_in_measure: | ||
| err_msgs.append( | ||
| f"There are {n_options_in_cfg} options in an upgrade in your project file and only " | ||
| f"{n_options_in_measure} in the ApplyUpgrade measure." | ||
| ) | ||
| if n_costs_in_cfg > n_costs_per_option_in_measure: | ||
| err_msgs.append( | ||
| f"There are {n_costs_in_cfg} costs on an option in your project file and only " | ||
| f"{n_costs_per_option_in_measure} in the ApplyUpgrade measure." | ||
| ) |
There was a problem hiding this comment.
Write some errors that tell them what's going on if there's going to be a problem.
| if err_msgs: | ||
| err_msgs.append( | ||
| "See https://github.com/NREL/buildstockbatch/wiki/Adding-Options-to-the-ApplyUpgrade-measure" | ||
| ) | ||
| raise ValidationError("\n".join(err_msgs)) |
There was a problem hiding this comment.
Send them to a page on the wiki explaining how to fix the problem.
| cfg = get_project_configuration(str(proj_filename)) | ||
| cfg["upgrades"][-1]["options"] = cfg["upgrades"][-1]["options"] * 10 | ||
| cfg["upgrades"][0]["options"][0]["costs"] = cfg["upgrades"][0]["options"][0]["costs"] * 5 |
There was a problem hiding this comment.
In the test I add a lot more options so we bump into the validation error.
| @@ -68,6 +68,7 @@ def run_tests(self): | |||
| 'yamale', | |||
| 'ruamel.yaml', | |||
| 'awsretry', | |||
| 'lxml' | |||
There was a problem hiding this comment.
We need lxml to parse the xml. If adding yet another kind of heavy dependency gives anybody heartburn, I could begrudgingly rewrite this to just use element tree parser in the standard library.
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against bbaeb14 |
| if os.path.exists(measure_xml_filename): | ||
| measure_xml_tree = objectify.parse(measure_xml_filename) | ||
| measure_xml = measure_xml_tree.getroot() | ||
| n_options_in_measure = 0 | ||
| n_costs_per_option_in_measure = 0 | ||
| for argument in measure_xml.arguments.argument: | ||
| m_option = re.match(r"^option_(\d+)$", str(argument.name)) | ||
| if m_option: | ||
| option_number = int(m_option.group(1)) | ||
| n_options_in_measure = max(option_number, n_options_in_measure) | ||
| m_costs = re.match(r"^option_(\d+)_cost_(\d+)_value", str(argument.name)) | ||
| if m_costs: | ||
| cost_number = int(m_costs.group(2)) | ||
| n_costs_per_option_in_measure = max(cost_number, n_costs_per_option_in_measure) |
There was a problem hiding this comment.
Would it speed things up to not check every option's number of costs (since we know it's the same number for every option)?
| @@ -79,4 +79,4 @@ jobs: | |||
| if: ${{ matrix.python-version == '3.10' }} | |||
| with: | |||
| name: documentation | |||
| path: docs/_build/html/ | |||
| path: buildstockbatch/docs/_build/html/ | |||
There was a problem hiding this comment.
Unrelated to this issue?
There was a problem hiding this comment.
Yes, unrelated. I missed it on a previous PR.
docs/changelog/changelog_dev.rst
Outdated
|
|
||
| .. change:: | ||
| :tags: validaton |
There was a problem hiding this comment.
Oops. Ha. Thanks for catching all my hasty typos.
docs/changelog/changelog_dev.rst
Outdated
| Adds some integration tests with the lastest ResStock. |
Fixes #329.
Pull Request Description
This checks the ApplyUpgrade measure and throws an error message with a link explaining how to fix it if there are more options than the measure allows.
Checklist
Not all may apply
minimum_coveragein.github/workflows/ci.ymlas necessary.Update validation for project config yaml file changesUpdate existing documentation