-
Notifications
You must be signed in to change notification settings - Fork 25
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
Feature/nl formulation #29
Conversation
…actual constraint value can be passed directly from app.py
Co-authored-by: Kate Culver <[email protected]>
Co-authored-by: Kate Culver <[email protected]>
Co-authored-by: Kate Culver <[email protected]>
@k8culver responded to your comments, let me know if you spot anything else. To your other concerns:
It doesn't perform well under the default time limit. Originally, I used a quick heuristic to enter an explicit time limit (the greatest dimension of the
I agree with this. It also happens using CQM. My current understanding is that the cell styling built in
There is currently no conditional styling for an unavailable shift that is assigned. At the moment, these shifts aren't even visible as assigned; they just show an error message and are not colored blue. Might be something to fix and add in your PR. Maybe check with Victoria first to ask if this was intentionally omitted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for those changes! Overall looks great, just going to wait to approve till this is integrated with the new settings to remind myself to give it one last look over. Also curious to see if Alex has any ideas for improving the solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looked at the formulation, only poked around the other code. Formulation seems reasonable to me
@@ -35,6 +38,48 @@ | |||
DAYS = ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"] | |||
WEEKEND_IDS = ["1", "7", "8", "14"] | |||
|
|||
@dataclass | |||
class ModelParams: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure that this class makes the code more readable relative to the old keyword argument approach. I need to now inspect two different files to understand what is being passed to the function.
Defer to @k8culver though for overall design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a pain but I do like how MVRP uses a class as seen in this file https://github.com/dwave-examples/mvrp/blob/main/solver/cvrp.py also this file https://github.com/dwave-examples/mvrp/blob/main/solver/solver.py
But this might be scope creep/overkill.
I do like the way everything is labeled with Jameson's solution (I was updating the tests in my branch and it was a pain without knowing which variable was which) but I agree that as it currently stands it's kind of hiding the parameters. I would say at the bare minimum it would be good to have keyword arguments or another way to label the parameters.
If either of you have other suggestions let me know!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A middle ground between @jlalbers 's approach and what's already there would be
from dataclasses import dataclass, asdict
@dataclass
class ModelParams:
param1: int = 1
param2: str = "hi there"
def func(param1, param2):
print(param1, param2)
func(**asdict(ModelParams()))
it could make sense to do ^ for this PR, then make the deeper class restructure as a followup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the best balance between function readability and ease of defining parameters. Will rework this in a new commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the example to implement @arcondello 's strategy
# Create availability constant | ||
availability_list = [params.availability[employee] for employee in employees] | ||
for i, sublist in enumerate(availability_list): | ||
availability_list[i] = [a if a != 2 else 100 for a in sublist] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why 2
, why 100
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of the Dash app, where
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment to add context to this
Co-authored-by: Kate Culver <[email protected]>
Co-authored-by: Alexander Condello <[email protected]>
@arcondello any ideas regarding the time limit? As @k8culver mentioned, the default time limit doesn't do the NL solver any favours for this particular problem. |
No great wisdom unfortunately. If we have to derive it empirically based on performance then I guess that's what we have to do 🤷 |
Could also be that using a binary symbol isn't the most performant modelling choice. I might also try a set-based approach and see if that works better. |
fc889c3
to
95aeacd
Compare
# Make every employee available for every shift | ||
availability = { | ||
"A-Mgr": [1] * 14, | ||
"B-Mgr": [1] * 14, | ||
"C": [1] * 14, | ||
"D": [1] * 14, | ||
"E": [1] * 14, | ||
"E-Tr": [1] * 14, | ||
} | ||
|
||
state = asarray([ | ||
# Give managers alternating shifts | ||
[i % 2 for i in range(14)], | ||
[(i+1) % 2 for i in range(14)], | ||
[1 for _ in range(14)], | ||
[1 for _ in range(14)], | ||
[1 for _ in range(14)], | ||
[1 for _ in range(14)], | ||
]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@k8culver noticed that the availability for this test specifies 14 shifts, but the state originally only described 5 shifts. I fixed it here, but this error is also present in test_build_from_sample
. Can fix it there as well or wait for your PR to be merged. Also could be a question of the robustness of this specific test if this mismatch is allowed to occur without errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I initially broke the tests with my PR implementing 14 day schedules because I forgot to update the tests and then I think Theodor updated them to fix my forgetfulness in this PR. So I think it's just a mistake (but that's a good point that it's a bit concerning that it works with that mismatch). Feel free to make any changes to make the tests make more sense or if you want me too I can instead.
@jlalbers I'm sorry it took so long but my changes are merged now so let me know if you need any help resolving merge conflicts! |
@k8culver no worries, I'll rebase my branch and make sure I include the new features! |
@k8culver thanks for taking over for me! |
Nonlinear solver formulation and implementation for the employee scheduling demo. Update summary below:
max_consecutive_shifts
can be passed directly torun_cqm
instead of having to passmax_consecutive_shifts + 1
dwave-optimization>=0.3.0
to requirements.txt to allow scalar constant broadcastingRequesting input on the following items:
assignments
BinaryVariable but there might be a better way to do it. Currently the default time limit is used.