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

Feature/nl formulation #29

Closed
wants to merge 49 commits into from

Conversation

jlalbers
Copy link

Nonlinear solver formulation and implementation for the employee scheduling demo. Update summary below:

  • Nonlinear formulation of the employee scheduling problem featured in the demo
  • Ability to select CQM or Nonlinear solver to run the demo
  • ModelParams dataclass to simplify passing of parameters to model creation/running functions
  • Update CQM manager constraint formulation so that max_consecutive_shifts can be passed directly to run_cqm instead of having to pass max_consecutive_shifts + 1
  • Add dwave-optimization>=0.3.0 to requirements.txt to allow scalar constant broadcasting

Requesting input on the following items:

  • Any other input on the formulation? After the formulation review I attempted to implement the array-based constraint for isolated days off as suggested, but the solver struggled with feasibility for this constraint even using long time limits so I reverted to the previous formulation. The only other change was boosting the objective value of preferred shifts from 2 to 100 which helped the solver assign more preferred shifts with less runtime.
  • Should we stick with the default time limit for NL or use some heuristic to calculate an appropriate one? I find that the NL solver struggles to find good solutions with the default time limit. I played around with using a heuristic assigning the time limit to the largest dimension of the assignments BinaryVariable but there might be a better way to do it. Currently the default time limit is used.
  • Should we allow the user to select a time limit when running the NL version? If so, should we also allow a time limit when running CQM? CQM performs well with the default time limit but NL struggles.

@jlalbers jlalbers requested a review from vgoliber as a code owner September 18, 2024 18:31
@jlalbers
Copy link
Author

@k8culver responded to your comments, let me know if you spot anything else. To your other concerns:

The solutions aren't great in comparison to the CQM solutions. Maybe Alex has some suggestions but for it's current performance it probably isn't great showing it against the CQM. If performance can't be improved I would suggest moving the setting for which solver to the configs file so there's less comparison.

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 assignments decision variable) but left it out of the PR initially for @arcondello 's suggestions. Using that heuristic, it performs well on smaller schedules (with slightly longer runtime than CQM) but chugs a bit if you max out the number of employees. We could also try seeding it with an initial state and see if that helps.

Also I think this is an existing bug but if an employee is unavailable for a shift that specific shift isn't in blue or any color which is probably not good. I would probably prefer it in solid red or blue with a red outline. See the error for Zachary in this screenshot (edit: also there's a chance this was intentional, I can see reasoning for it, I don't particularly agree with the reasoning but I can understand it 😂 ):

I agree with this. It also happens using CQM. My current understanding is that the cell styling built in utils.display_schedule only styles based on the following criteria:

  • row_index is odd
  • column_id is a weekend id
  • dataframe element == " " (assigned shift)
  • requested shift is not assigned

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.

Copy link
Collaborator

@k8culver k8culver left a 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.

app.py Outdated Show resolved Hide resolved
app.py Outdated Show resolved Hide resolved
app_html.py Show resolved Hide resolved
Copy link
Contributor

@arcondello arcondello left a 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:
Copy link
Contributor

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.

Copy link
Collaborator

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!

Copy link
Contributor

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?

Copy link
Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good! Thank you!

Copy link
Author

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

employee_scheduling.py Outdated Show resolved Hide resolved
# 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]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why 2, why 100?

Copy link
Author

@jlalbers jlalbers Sep 20, 2024

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 $a_{e,s}$ is employee $e$'s availability for shift $s$, a value of $a_{e,s}=0$ is assigned if the employee is unavailable for that shift, $a_{e,s}=1$ if they are available, and $a_{e,s}=2$ if the employee prefers that shift. These are the values in the assignments parameter that is passed into the model-building function. The objective for the model is then calculated as $\sum_e^{|E|}\sum_s^{|S|}a_{e,s}x_{e,s}, \hspace{2mm} x\in \{0,1\} $, where $x_{e,s}$ is the decision variable representing employee $e$'s assignment to shift $s$. In my experimentation, I found that boosting the value of $a_{e,s}$ to 100 from 2 helped the NL solver find better solutions faster since assignment of a preferred shift would have a greater impact on the objective value. The exact value of 100 is somewhat arbitrary though, I just found that a value of 10 didn't help the speedup much and 1,000 performed about as well as 100. I can add a comment to explain this so it doesn't come out of nowhere.

Copy link
Author

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

@jlalbers
Copy link
Author

@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.

@arcondello
Copy link
Contributor

No great wisdom unfortunately. If we have to derive it empirically based on performance then I guess that's what we have to do 🤷

@jlalbers
Copy link
Author

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.

@jlalbers jlalbers force-pushed the feature/nl-formulation branch from fc889c3 to 95aeacd Compare September 20, 2024 20:27
Comment on lines +280 to +298
# 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)],
])
Copy link
Author

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.

Copy link
Collaborator

@k8culver k8culver Sep 20, 2024

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.

@k8culver
Copy link
Collaborator

@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!

@jlalbers
Copy link
Author

@k8culver no worries, I'll rebase my branch and make sure I include the new features!

@k8culver k8culver mentioned this pull request Dec 21, 2024
@k8culver
Copy link
Collaborator

Closing in favour of PR #34. Thank you @jlalbers!

@k8culver k8culver closed this Dec 24, 2024
@jlalbers
Copy link
Author

@k8culver thanks for taking over for me!

@k8culver
Copy link
Collaborator

@k8culver thanks for taking over for me!

@jlalbers Thanks for doing 99% of the work! 🙌

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.

3 participants