Skip to content

Conversation

@Lingyan90
Copy link
Contributor

Fixes

Fix training issue: bug in subcritical powerplant report method #1287.

Also updated the tags units to remove duplicate units in output plant_pdf_results.svg.

Summary/Motivation:

Reproduced the KeyError issue in local machine. The second error regarding solver "IPOPT" was not observed when testing in local machine. This PR helps resolve the KeyError mentioned in the issue.

Changes proposed in this PR:

  • Changed the tags method when print_pfd_results.
  • Allow tags only when the components is existing in that stream.
  • Removed duplicated units in plant_pdf_results.svg.
  • Removed incorrect unit conversion of /1000.

Legal Acknowledgement

By contributing to this software project, I agree to the following terms and conditions for my contribution:

  1. I agree my contributions are submitted under the license terms described in the LICENSE.txt file at the top level of this directory.
  2. I represent I am authorized to make the contributions and grant the license. If my employer has rights to intellectual property that includes these contributions, I represent that I have received permission to make contributions and grant the required license on behalf of that employer.

the change tags components that exist in the stream, e.g. for flue-gas streams, it tags N2, O2, NO, etc.; for water/steam streams, it only tags H2O, as the IAPWS95 property package only contains water.
removed duplicate pressure tag; removed dividend by 1000 for hmass and pressure as their unit are in J/kg and Pa, respectively
@Lingyan90 Lingyan90 self-assigned this Jan 8, 2026
@codecov
Copy link

codecov bot commented Jan 8, 2026

Codecov Report

❌ Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.74%. Comparing base (30cbaeb) to head (feb527d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...subcritical_power_plant/subcritical_power_plant.py 94.11% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1720      +/-   ##
==========================================
+ Coverage   73.66%   73.74%   +0.08%     
==========================================
  Files         397      398       +1     
  Lines       65041    65094      +53     
  Branches    10949    10954       +5     
==========================================
+ Hits        47910    48002      +92     
+ Misses      14628    14588      -40     
- Partials     2503     2504       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@dallan-keylogic dallan-keylogic left a comment

Choose a reason for hiding this comment

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

LGTM

Edit: except that the changes aren't being tested. A way you could test these changes is by extracting lines 2211 to 2238 into a helper function that returns the tags and tags_formats dictionaries, then test the outputs of that function.

@ksbeattie ksbeattie added the Priority:Normal Normal Priority Issue or PR label Jan 15, 2026
@dallan-keylogic
Copy link
Contributor

@Lingyan90, can you get to adding tests before the February release?

@Lingyan90
Copy link
Contributor Author

Lingyan90 commented Jan 29, 2026

LGTM

Edit: except that the changes aren't being tested. A way you could test these changes is by extracting lines 2211 to 2238 into a helper function that returns the tags and tags_formats dictionaries, then test the outputs of that function.

The original testing code didn't cover the info from line 2211 to 2238. The exiting testing code is neat and concise. My changes from line 2211 to line 2224 are mainly correct the unit shown in plot plant_pdf_results.svg, and line 2227-2230 for make sure the table report bug got fixed. As those didn't affect the test file, so I didn't make any changes to the test file.

@dallan-keylogic
Copy link
Contributor

The original testing code didn't cover the info from line 2211 to 2238. The exiting testing code is neat and concise. My changes from line 2211 to line 2224 are mainly correct the unit shown in plot plant_pdf_results.svg, and line 2227-2230 for make sure the table report bug got fixed. As those didn't affect the test file, so I didn't make any changes to the test file.

When we fix issues that were not covered by our current tests, we like to write new tests to verify that the functionality does not break in the future. That way we can increase the overall test coverage of the repository.

Lingyan90 and others added 6 commits February 2, 2026 13:45
modified the subcritical_power_plant.py with healer function for tags, tag formats, and tag_group to support a cleaner and simpler tags, tag formats, and tag_group test in the test file.
…hod' of https://github.com/Lingyan90/idaes-pse-lyd into fix-issue_1287-bug-in-subcritical-powerplant-report-method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority:Normal Normal Priority Issue or PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants