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

Resource tasks distance #87

Merged
merged 4 commits into from
Jul 29, 2021
Merged

Resource tasks distance #87

merged 4 commits into from
Jul 29, 2021

Conversation

tpaviot
Copy link
Owner

@tpaviot tpaviot commented Jul 29, 2021

Related to #83

Please @dreinon can you test

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2021

Codecov Report

Merging #87 (0aca000) into master (c821577) will increase coverage by 0.22%.
The diff coverage is 98.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #87      +/-   ##
==========================================
+ Coverage   94.96%   95.19%   +0.22%     
==========================================
  Files          30       31       +1     
  Lines        3419     3643     +224     
==========================================
+ Hits         3247     3468     +221     
- Misses        172      175       +3     
Impacted Files Coverage Δ
processscheduler/__init__.py 87.50% <ø> (ø)
processscheduler/resource_constraint.py 96.11% <94.73%> (-0.86%) ⬇️
test/test_resource_tasks_distance.py 99.46% <99.46%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c821577...0aca000. Read the comment docs.

@dreinon
Copy link
Contributor

dreinon commented Jul 29, 2021

Please test, there seems to be an issue with the task duration (space is 6 whereas it should be 8).

Hey Thomas, were you able to fix this issue?

@dreinon
Copy link
Contributor

dreinon commented Jul 29, 2021

def test_resource_tasks_distance_5(self) -> None:
        """Adding one scheduled optional tasks"""
        pb = ps.SchedulingProblem("ResourceTasksDistance5OptionalTasks", horizon=30)
        task_1 = ps.FixedDurationTask("task1", duration=8)
        task_2 = ps.FixedDurationTask("task2", duration=4)
        task_3 = ps.FixedDurationTask("task3", duration=3, optional=True)
        worker_1 = ps.Worker("Worker1")
        task_1.add_required_resource(worker_1)
        task_2.add_required_resource(worker_1)
        task_3.add_required_resource(worker_1)

        c1 = ps.ResourceTasksDistance(worker_1, distance=4, mode="exact")
        pb.add_constraint(c1)

        pb.add_constraint(ps.TaskStartAt(task_1, 1))

        solver = ps.SchedulingSolver(pb)
        # for optional tasks to not be scheduled
        solver.add_constraint(task_3.scheduled == True)

        solution = solver.solve()

        self.assertTrue(solution)

        t1_start = solution.tasks[task_1.name].start
        t2_start = solution.tasks[task_2.name].start
        t3_start = solution.tasks[task_3.name].start
        t1_end = solution.tasks[task_1.name].end
        t2_end = solution.tasks[task_2.name].end
        t3_end = solution.tasks[task_3.name].end
        self.assertEqual(t1_start, 1)
        self.assertEqual(t1_end, 9)
        self.assertTrue(t2_start == 13 or t3_start == 13)
        if t2_start == 13:
            self.assertEqual(t2_end, 17)
            self.assertEqual(t3_start, 21, f'{t3_start=} {t3_end=}')
            self.assertEqual(t3_end, 24)
        elif t3_start == 13:
            self.assertEqual(t3_end, 16)
            self.assertEqual(t2_start, 20)
            self.assertEqual(t2_end, 24)

Testing with scheduled optional task and working as expected!!

@dreinon
Copy link
Contributor

dreinon commented Jul 29, 2021

The next step for this constraint was to make it admit periods, so the distance is applied inside certain periods and not outside of them.
For example, resource A tasks distance should be 4 in periods [(0,20), (23, 40)].
We have to decide if each constraint should admit a single period, and in order to do the example, 2 constraints would need to be added, one for (0,20) and another one for (23, 40); or if each constraint can deal with multiple periods like in the example.

Thanks!

@tpaviot
Copy link
Owner Author

tpaviot commented Jul 29, 2021

@dreinon Yes, it works with optional tasks as well. That should not be very hard to add a time restriction to this constraint, although I don't really understand the use case. The documentation should explicit this clearly, how to use the constraint and what kind of problem this constraint represents.

@dreinon
Copy link
Contributor

dreinon commented Jul 29, 2021

Right, I can doc it myself.

My use case is that in my problem, I have several periods that are not consecutive one from each other, for example, from 9h to 13:30h, then from 14h to 16:15h and then from 17:15h to 21h, all with 45 min task duration.
In real life, these three periods are clearly separated by time, although in the ProcessScheduler problem representation, since we are dealing with integers, 9h to 13:30h would be (0,6), then 14h to 16:15h would be (6,9) and then 17:15h to 21h would be (9,14).

This is modelled this way for 3 reasons:

  1. If we considered "rest spaces" in the problem, then all resources would be unavailable there and it would only add complexity to the problem enlarging horizon.
  2. "Rest spaces" might not be split in 45 min. periods, therefore we wouldn't be able to represent them correctly in the problem.
  3. We tried setting granularity to be 15 min. so we could represent these "rest periods too", but it made the problem way too complex, having horizons of around 300.

Therefore, if the constraints didn't have periods, then distance wouldn't be real. Imagine now the end of a day and the start of another day of the week. They share the same integer timepoint even though they are really apart in real time.
Imagine an equal distance of 0 between tasks of the same resource constraint is used without specifying periods. Then, a possible case would be that one task ended a day and another one started a day, and that would have distance 0 in the problem, which wouldn't work for real life because those two periods are actually not even close.

I hope I could explain my use case well and that you get what I was trying to explain :)

@tpaviot
Copy link
Owner Author

tpaviot commented Jul 29, 2021

ok, I will add a time_periods parameter, for example time_periods=[(0,6), (6,9), (9,14)] in which the distance constraint applies

@dreinon
Copy link
Contributor

dreinon commented Jul 29, 2021

Thanks!

@sourcery-ai
Copy link

sourcery-ai bot commented Jul 29, 2021

Sourcery Code Quality Report

❌  Merging this PR will decrease code quality in the affected files by 1.67%.

Quality metrics Before After Change
Complexity 9.77 🙂 10.21 🙂 0.44 👎
Method Length 77.67 🙂 96.12 🙂 18.45 👎
Working memory 11.02 😞 10.94 😞 -0.08 👍
Quality 53.25% 🙂 51.58% 🙂 -1.67% 👎
Other metrics Before After Change
Lines 212 281 69
Changed files Quality Before Quality After Quality Change
processscheduler/init.py 83.93% ⭐ 83.77% ⭐ -0.16% 👎
processscheduler/resource_constraint.py 51.51% 🙂 50.47% 🙂 -1.04% 👎

Here are some functions in these files that still need a tune-up:

File Function Complexity Length Working Memory Quality Recommendation
processscheduler/resource_constraint.py WorkLoad.__init__ 13 🙂 257 ⛔ 14 😞 35.94% 😞 Try splitting into smaller methods. Extract out complex expressions
processscheduler/resource_constraint.py ResourceTasksDistance.__init__ 14 🙂 223 ⛔ 12 😞 39.95% 😞 Try splitting into smaller methods. Extract out complex expressions

Legend and Explanation

The emojis denote the absolute quality of the code:

  • ⭐ excellent
  • 🙂 good
  • 😞 poor
  • ⛔ very poor

The 👍 and 👎 indicate whether the quality has improved or gotten worse with this pull request.


Please see our documentation here for details on how these metrics are calculated.

We are actively working on this report - lots more documentation and extra metrics to come!

Help us improve this quality report!

@tpaviot
Copy link
Owner Author

tpaviot commented Jul 29, 2021

I committed an update with time periods, that should work. You can have a look at the related unittest as well, see 0aca000

@tpaviot
Copy link
Owner Author

tpaviot commented Jul 29, 2021

Sourcery is good, but a bit too much intrusive

@tpaviot tpaviot merged commit b89b842 into master Jul 29, 2021
@dreinon
Copy link
Contributor

dreinon commented Jul 29, 2021

Sourcery is good, but a bit too much intrusive

Right, I like that but I understand it's not for everybody 😄

Thanks!

@tpaviot tpaviot deleted the resource-tasks-distance branch August 2, 2021 06:05
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.

None yet

3 participants