Skip to content

Make exporting of PyROS subproblems more customizable #3649

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

Merged
merged 11 commits into from
Jul 21, 2025

Conversation

shermanjasonaf
Copy link
Contributor

@shermanjasonaf shermanjasonaf commented Jun 27, 2025

Summary/Motivation:

The PyROS solver provides two optional arguments keepfiles and subproblem_file_directory for specifying whether and where subproblems that were not solved to an acceptable level are to be written. However, when subproblems are written, they are exclusively written in 'bar' format. Other formats ('gams', 'nl', etc.) may be of interest to the user.

Changes proposed in this PR:

  • Add a new optional PyROS solver argument subproblem_format_options. This argument should be (castable to) a dict object, of which each entry maps a Pyomo WriterFactory format (e.g., 'bar', 'gams') to a value for the argument io_options to BlockData.write().
  • Add tests to check that PyROS writes subproblems as expected.

TODO (after #3646 merged)

  • Update version number, changelog

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.

Copy link

codecov bot commented Jun 29, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.07%. Comparing base (14a1c10) to head (425e76c).
Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3649   +/-   ##
=======================================
  Coverage   89.06%   89.07%           
=======================================
  Files         890      890           
  Lines      102700   102696    -4     
=======================================
+ Hits        91471    91472    +1     
+ Misses      11229    11224    -5     
Flag Coverage Δ
builders 26.64% <33.33%> (+<0.01%) ⬆️
default 85.66% <100.00%> (?)
expensive 34.00% <33.33%> (?)
linux 86.84% <100.00%> (-1.98%) ⬇️
linux_other 86.84% <100.00%> (+0.03%) ⬆️
osx 83.13% <100.00%> (+0.03%) ⬆️
win 85.02% <100.00%> (+0.02%) ⬆️
win_other 85.02% <100.00%> (+0.02%) ⬆️

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

☔ 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

@mrmundt mrmundt left a comment

Choose a reason for hiding this comment

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

I love reductions of duplicate code. Looks good!

@jsiirola
Copy link
Member

@shermanjasonaf: Both dependent PRs (#3558 & #3646) have been merged. Once the version # and changelog have been updated, we can merge this one as well.

Comment on lines -1053 to +1056
Iterations : 4
Solve time (wall s) : 5.281
Iterations : 5
Solve time (wall s) : 8.824
Copy link
Member

Choose a reason for hiding this comment

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

Should we be concerned that the number of iterations has increased?

Copy link
Contributor Author

@shermanjasonaf shermanjasonaf Jul 21, 2025

Choose a reason for hiding this comment

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

Probably. After a more detailed look, I have found that, for the problem in question, the termination stats are sensitive to the subsolver. The results prior to the change were obtained using BARON 25.2.1 (no CPLEX access) as the PyROS subsolver; the results after the change were obtained using BARON 25.2.1 (w/ CPLEX 22.1.0 access) as the PyROS subsolver. See the table below for more detailed results, noting that in all prior PyROS versions, the final objective value that was reported is 3.6285E+07 (other values in the table are bold). In the future, we may want to modify the problem to one that is less sensitive to the subsolvers. Perhaps, for now, we should note somewhere in the docs the subsolver version that was used. Your thoughts?

Subsolver PyROS Termination Condition Iterations Final Objective
SCIP 9.1.0 (+SOPLEX 7.1.0) robust optimal 10 3.6107E+07
BARON 25.2.1 (no CPLEX) robust optimal 4 3.6285E+07
BARON 25.2.1 (w/ CPLEX) robust optimal 5 3.6285E+07
BARON 25.3.19 (no CPLEX) robust optimal 6 3.6285E+07
BARON 25.3.19 (w/ CPLEX) subsolver error 7 3.6285E+07
BARON 25.7.17 (no CPLEX) robust optimal 7 3.6103E+07
BARON 25.7.17 (w/ CPLEX) subsolver error 12 3.6286E+07

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I like the idea of moving to another problem in the future that is a little more numerically stable/robust. I don't think this should hold up this PR, do you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this PR should not be held up by these findings, as the findings are not related to the functional changes of the PR.

@jsiirola jsiirola merged commit 8dfd55b into Pyomo:main Jul 21, 2025
35 checks passed
@github-project-automation github-project-automation bot moved this from Reviewer Approved to Done in August 2025 Release Jul 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

4 participants