Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix create compsets heron function 2 #19

Conversation

caleb-sitton-inl
Copy link
Collaborator

@caleb-sitton-inl caleb-sitton-inl commented Jun 25, 2024


Pull Request Description

What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)

#17 #18

What are the significant changes in functionality due to this change request?

To address issue #17, the src/heron/create_componentsets_in_HERON() was updated such that CashFlow subnodes that need updating are no longer immediately removed within the for loop that iterates over all of the CashFlow's children. Instead, they are added to a list. An additional for loop is then run, iterating over the new list, to remove each CashFlow child node that needs to be updated.

The testing of the above change revealed an error in the tests/integration_tests/HERON_update_withCompSets/gold/ new_heron_input.xml file. As described in issue #17, the gold file contains duplicate subnodes of a CashFlow (namely, two <scaling_factor_x> subnodes) instead of a single, updated version of the subnode.

To address issue #18, the line in the create_componentsets_in_HERON() function containing the json.load() function has been placed within a try/except statement. If the json.load() function fails to properly decode the file (e.g., the file is not correctly formatted as a JSON), the rather cryptic error it raises will be caught. Instead, a custom ValueError with a clearer explanation of the problem is raised. In addition, the conditional that filters which files in the input component sets folder the function attempts to read was updated. Now, only .json and .txt files in the input component sets folder are allowed past this filter, since all component set data should be in a file that is one of these two types.

To correct initial test failure: The same issue was present in the tests/integration_tests/Aspen_HERON_FullTest/gold/ new_heron_input.xml file as was present in the HERON_update_withCompSets gold file mentioned above. In this case, duplicate <reference_driver> nodes were present in the gold file.


For Change Control Board: Change Request Review

The following review must be completed by an authorized member of the Change Control Board.

  • 1. Review all computer code.
  • 2. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
  • 3. Automated Tests should pass, including run_tests, pylint, and manual building tests.
  • 4. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large test.
  • 5. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
  • 6. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.

@GabrielSoto-INL GabrielSoto-INL self-requested a review June 26, 2024 15:41
Copy link
Collaborator

@GabrielSoto-INL GabrielSoto-INL left a comment

Choose a reason for hiding this comment

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

some additional changes are requested, otherwise great work finding that error

src/heron.py Outdated
textfile_path = comp_sets_folder+"/"+textfile
comp_set_dict = json.load(open(textfile_path))
try:
comp_set_dict = json.load(open(textfile_path))
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be better to open the file using a with statement as mentioned here: https://pylint.readthedocs.io/en/latest/user_guide/messages/refactor/consider-using-with.html

src/heron.py Outdated
try:
comp_set_dict = json.load(open(textfile_path))
except json.JSONDecodeError as e:
raise ValueError(f"The content of {textfile_path} is not in proper JSON format and cannot be read")
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is good, you may want to add a from e at the end of this line. this ensures that your error message appears as well as the original JSON error (more info: https://docs.python.org/3/tutorial/errors.html#exception-chaining)

@@ -49,9 +49,6 @@
<driver>
<Function method="capacity">functions</Function>
</driver>
<reference_driver>
<fixed_value>10.0</fixed_value>
Copy link
Collaborator

Choose a reason for hiding this comment

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

so essentially, these were the original reference drivers that instead of their values getting updated, a new node was added and the old one remained?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly. The design was that any and all reference_driver, reference_price, and scaling_factor_x nodes were cleared, then all three were added; however, the old nodes were not all deleted.

Copy link
Collaborator

@GabrielSoto-INL GabrielSoto-INL left a comment

Choose a reason for hiding this comment

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

Changes are good to merge

@GabrielSoto-INL GabrielSoto-INL merged commit 6a16375 into idaholab:main Jun 26, 2024
1 check passed
@caleb-sitton-inl caleb-sitton-inl deleted the fix-create-compsets-heron-function-2 branch June 26, 2024 18:43
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