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

Adds support for the validation of IndexedSets using the index #3304

Closed
wants to merge 20 commits into from

Conversation

pmlpm1986
Copy link
Contributor

Fixes #2655.

Summary/Motivation:

Set members could not be validated using the index in indexed sets. This PR adds that option to allow for more complex validation rules.

Changes proposed in this PR:

  • Allows validation of indexed sets to access the member and the respective index just like in Parameter objects
  • Supports one- and multi-dimensional set members
  • Adds tests to retain the functionality
  • Revised the tutorial to illustrate the functionality.

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.

@blnicho blnicho requested a review from jsiirola July 2, 2024 19:02
pyomo/core/base/set.py Show resolved Hide resolved
pyomo/core/base/sets.py Outdated Show resolved Hide resolved
pyomo/core/tests/unit/test_set.py Outdated Show resolved Hide resolved
pyomo/core/tests/unit/test_sets.py Outdated Show resolved Hide resolved
@pmlpm1986 pmlpm1986 reopened this Jul 17, 2024
@pmlpm1986
Copy link
Contributor Author

I reverted the changes introduced by black. It should work as intended now.

pyomo/core/base/set.py Outdated Show resolved Hide resolved
@@ -4319,15 +4319,15 @@ def _lt_3(model, i):

m = ConcreteModel()

def _validate(model, i, j):
def _validate_I(model, i, j):
Copy link
Contributor

Choose a reason for hiding this comment

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

I envision that future developers looking at this code will wonder, "What is I?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my mind, it is a simple set that we validate through the normal rule (no index).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is, a non-indexed set.

@@ -4347,7 +4347,10 @@ def _validate(model, i, j):

# Note: one of these indices will trigger the exception in the
# validot when it is called for the index.
m.J = Set([(0, 0), (2, 2)], validate=_validate)
def _validate_J(model, i, j, index):
Copy link
Contributor

Choose a reason for hiding this comment

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

I envision that future developers looking at this code will wonder, "What is J?"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my mind, it is just an indexed set that we validate through this new rule.

@jsiirola
Copy link
Member

This is pretty close. I really only have two concerns with it:

  • I would prefer that this leverage the Initializer() logic if at all possible: that logic standardizes how we process arguments to Component constructors so that (hopefully) all Components / arguments behave the same.
  • (and this is the bigger issue): this PR breaks backwards compatibility. That is, older models (including the tests in test_sets) no longer work with this change. Although The Book is ambiguous as to the signature that IndexedSet.validate should take, the fact that there are examples / tests that exercise the old convention makes me uneasy about breaking it. I think we should update this PR to either
    1. Work (silently) with either syntax
    2. Expect the new syntax but accept the old syntax (issuing a deprecation warning)

I am happy to work on a patch for this, but it will take me a little while to get to it.

@pmlpm1986
Copy link
Contributor Author

This is pretty close. I really only have two concerns with it:

* I would prefer that this leverage the `Initializer()` logic if at all possible: that logic standardizes how we process arguments to Component constructors so that (hopefully) all Components / arguments behave the same.

* (and this is the bigger issue): this PR breaks backwards compatibility.  That is, older models (including the tests in `test_sets`) no longer work with this change.  Although The Book is ambiguous as to the signature that `IndexedSet.validate` should take, the fact that there are examples / tests that exercise the old convention makes me uneasy about breaking it.  I think we should update this PR to either
  
  1. Work (silently) with either syntax
  2. Expect the new syntax but accept the old syntax (issuing a deprecation warning)

I am happy to work on a patch for this, but it will take me a little while to get to it.

As you say, the book is vague about this and it led me to think Sets behaved exactly like Parameters. Once I realised that they did not, I raised this issue. Only recently did I find the time to come up with a solution. Therefore, the Initializer() approach makes sense but I never looked into it.

On the subject of backwards compatibility, the adjustments needed are minimal and by design: all that is needed is to add the index to the arguments (it is always the last argument) and ignore it, which is what I did in some of the tests. Issuing a deprecation warning seems reasonable.

@jsiirola
Copy link
Member

This PR has been superseded by #3338 (that PR includes all commits here plus the rework of the Initializer and restoration of backwards compatibility [with deprecation warnings]). I am closing this PR to ease some of the burden on the CI system.

@jsiirola jsiirola closed this Aug 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

No access to the index when validating indexed set members
4 participants