pyomo icon indicating copy to clipboard operation
pyomo copied to clipboard

Modified _parse_gdx_results in GAMS.py to replace _parse_special_value

Open AnhTran01 opened this issue 6 months ago • 6 comments

Fixes #3624

Summary/Motivation:

When parsing the results of a model after it has been solved, the level and dual value are obtained through a series of if statements in _parse_special_values that may cause slowdowns. This PR added GAMS existing functions to handle data parser for these special values in _parse_gdx_results.

Changes proposed in this PR:

  • Replaced _parse_special_values with GAMS special value parser in _parse_gdx_results.

Legal Acknowledgement

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

  1. I agree my contributions are submitted under the BSD license.
  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.

AnhTran01 avatar Jun 16 '25 12:06 AnhTran01

@boxblox @abhosekar

AnhTran01 avatar Jun 16 '25 12:06 AnhTran01

this is a small thing, but we should add gdx.gdxLibraryUnload() after the gdxFree(pgdx) calls in both the results file and the stats file read operations

boxblox avatar Jun 16 '25 16:06 boxblox

Codecov Report

Attention: Patch coverage is 86.17021% with 13 lines in your changes missing coverage. Please review.

Project coverage is 85.75%. Comparing base (ca33341) to head (e1da345).

Files with missing lines Patch % Lines
pyomo/solvers/plugins/solvers/GAMS.py 86.17% 13 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3629      +/-   ##
==========================================
- Coverage   88.93%   85.75%   -3.19%     
==========================================
  Files         888      888              
  Lines      102406   102435      +29     
==========================================
- Hits        91079    87844    -3235     
- Misses      11327    14591    +3264     
Flag Coverage Δ
builders 26.68% <15.95%> (-0.01%) :arrow_down:
default 85.58% <86.17%> (?)
expensive 34.04% <15.95%> (?)
linux ?
linux_other ?
osx ?
win ?
win_other ?

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

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

codecov[bot] avatar Jun 17 '25 04:06 codecov[bot]

It is unfortunate that e1da345 was committed... that introduced a bunch of non-functional changes that are confusing review of this PR. For the future, we run (and test against) black -C -S. If you want to make all the changes to the string quote characters, that's fine, but it would be better for that to be isolated to a separate PR that ONLY makes that change.

jsiirola avatar Jun 20 '25 21:06 jsiirola

This is my bad @jsiirola. In this case, would the best practice is to make a new PR on a clean branch with the changes to the import statement?

AnhTran01 avatar Jun 20 '25 21:06 AnhTran01

This is my bad @jsiirola. In this case, would the best practice is to make a new PR on a clean branch with the changes to the import statement?

That is completely up to you. You can just revert that commit on this branch and it is likely to be OK. If that still leaves "a mess" then you can decide if it is easier to just recreate a new branch / PR.

jsiirola avatar Jun 23 '25 16:06 jsiirola