Skip to content

Conversation

@shermanjasonaf
Copy link
Contributor

@shermanjasonaf shermanjasonaf commented Nov 14, 2025

Summary/Motivation:

This PR resolves issues introduced in #3341, to ensure that PyROS truly supports IntersectionSet instances representing intersections of uncertainty set sequences containing either of the following:

  • A set defined using "auxiliary" uncertain parameters (e.g., CardinalitySet, FactorModelSet)
  • A discrete set (e.g., DiscreteScenarioSet)

These use cases are now covered in the PyROS automated tests.

Changes proposed in this PR:

  • Fix IntersectionSet implementation for cases where discrete sets and/or sets involving auxiliary parameters are to be intersected
  • Add tests ensuring that the PyROS solver works for the above use cases
  • Tweak/fix and more extensively test several standard UncertaintySet base class attributes

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.

@codecov
Copy link

codecov bot commented Nov 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.43%. Comparing base (13facca) to head (3dd0dd6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3786      +/-   ##
==========================================
+ Coverage   89.40%   89.43%   +0.02%     
==========================================
  Files         909      909              
  Lines      105541   105579      +38     
==========================================
+ Hits        94364    94420      +56     
+ Misses      11177    11159      -18     
Flag Coverage Δ
builders 29.11% <41.17%> (+<0.01%) ⬆️
default 86.04% <100.00%> (?)
expensive 35.76% <41.17%> (?)
linux 86.74% <100.00%> (-2.44%) ⬇️
linux_other 86.74% <100.00%> (+0.01%) ⬆️
osx 82.89% <100.00%> (+0.02%) ⬆️
win 85.00% <100.00%> (+0.02%) ⬆️
win_other 85.00% <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.

@shermanjasonaf
Copy link
Contributor Author

@jas-yao @natalieisenberg

Copy link
Member

@jsiirola jsiirola left a comment

Choose a reason for hiding this comment

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

Looks great. One minor documentation request, but nothing I would hold up the PR for.

Returns
-------
: list of tuple
list of tuple
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, this would be a formal type:

Suggested change
list of tuple
list[tuple[float, float]]

(this comment applies below, too)

@github-project-automation github-project-automation bot moved this from Todo to Reviewer Approved in Pyomo 6.10 Nov 19, 2025
def scenarios(self):
"""
list of tuples : Uncertain parameter realizations comprising the
list of tuple : Uncertain parameter realizations comprising the
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this a formal return type (i.e., list[tuple[...]])?

(Same with other scenarios() below)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Reviewer Approved

Development

Successfully merging this pull request may close these issues.

4 participants