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

contrib.cp: Adding SequenceVar and other logical expressions for scheduling #3227

Merged
merged 54 commits into from
May 8, 2024

Conversation

emma58
Copy link
Contributor

@emma58 emma58 commented Apr 8, 2024

Fixes # .

Summary/Motivation:

This adds SequenceVar components and a number of associated and related expressions for modeling logical constraints on IntervalVars in constraint programming scheduling models. It also sneaks in a number of small bug fixes that I found along the way.

NOTE: this does sneak in a small change to core by removing a (very) old deprecation warning in Block.

Changes proposed in this PR:

  • Adds SequenceVar component to contrib.cp
  • Adds first_in_sequence, last_in_sequence, before_in_sequence, predecessor_to, and no_overlap expressions for SequenceVars
  • Adds alternative, spans, and synchronize expressions for scheduling logic
  • Fixes a bug to make References work for IntervalVarPresence, IntervalStartTime, and IntervalVarEndTime components
  • Moves the exit node handlers from the LogicalToDocplex writer onto the ExitNodeDispatcher base class in repn.util
  • Fixes a bug with pprinting precedence expressions with delays
  • Removes a deprecation warning from Pyomo 4.0 that IntervalVars would hit since they are often defined by multiple rule functions

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.

…e when I collect components in the docplex writer
…rs hit for convoluted reasons--but basically because they have no kwd args named 'rule'
Comment on lines -83 to 88
def __init__(self):
def __init__(self, *args, **kwd):
# TODO: adding args and kwd above made Reference work, but we
# probably shouldn't just swallow them, right?
super().__init__(ctype=IntervalVarPresence)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsiirola, I made this change to make Reference(interval_var[:].is_present) work, but I don't really understand what I did or know if this is the right fix.

Copy link
Member

Choose a reason for hiding this comment

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

I don't know why this was necessary. We should probably make a note to go back and look into why this was necessary. It makes me a bit nervous to accept and silently discard the arguments here.

Copy link

codecov bot commented Apr 8, 2024

Codecov Report

Attention: Patch coverage is 97.02970% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 88.41%. Comparing base (c68ce66) to head (071b017).
Report is 175 commits behind head on main.

Files Patch % Lines
pyomo/contrib/cp/repn/docplex_writer.py 94.44% 3 Missing ⚠️
pyomo/contrib/cp/sequence_var.py 96.25% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3227      +/-   ##
==========================================
+ Coverage   88.40%   88.41%   +0.01%     
==========================================
  Files         847      850       +3     
  Lines       95285    95459     +174     
==========================================
+ Hits        84232    84400     +168     
- Misses      11053    11059       +6     
Flag Coverage Δ
linux 86.36% <97.02%> (+0.02%) ⬆️
osx 76.23% <97.02%> (+0.04%) ⬆️
other 86.55% <97.02%> (+0.01%) ⬆️
win 83.86% <97.02%> (+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.

pyomo/contrib/cp/sequence_var.py Show resolved Hide resolved
m.iv = IntervalVar(
[1, 2, 3], start=start_rule, length=length_rule, optional=True
)
m.whole_enchilada = IntervalVar()
Copy link
Contributor

Choose a reason for hiding this comment

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

How dare you make me laugh like this.

pyomo/contrib/cp/tests/test_docplex_walker.py Outdated Show resolved Hide resolved
pyomo/contrib/cp/tests/test_interval_var.py Show resolved Hide resolved
pyomo/core/base/block.py Show resolved Hide resolved
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.

I have a number of questions / things we should look farther into, but I am OK deferring all of that to a subsequent PR


def __init__(self, *args, **kwargs):
self._init_rule = Initializer(kwargs.pop('rule', None))
self._init_expr = kwargs.pop('expr', None)
Copy link
Member

Choose a reason for hiding this comment

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

Why do you not use an Initializer here?

Comment on lines +81 to +84
if self._init_expr is not None and self._init_rule is not None:
raise ValueError(
"Cannot specify both rule= and expr= for SequenceVar %s" % (self.name,)
)
Copy link
Member

Choose a reason for hiding this comment

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

There is a method (self._pop_from_kwargs() that can combine this logic and the two pop() above. See constraint.py:660

Comment on lines +46 to +47
# We'll demand expr be a list for now--it needs to be ordered so this
# doesn't seem like too much to ask
Copy link
Member

Choose a reason for hiding this comment

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

This is technically not requiring a list (which is good). But, should there be a check for passing in a non-deterministic Sequence (e.g., set)?

pyomo/contrib/cp/sequence_var.py Show resolved Hide resolved
@mrmundt
Copy link
Contributor

mrmundt commented May 8, 2024

Jenkins failures are due to the license server for gurobi being unavailable. Merging.

@mrmundt mrmundt merged commit a44a832 into Pyomo:main May 8, 2024
29 of 31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants