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.

Sign up for GitHub

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

Jump to bottom

Scheduling move checks to find best index #227

Merged
merged 2 commits into from
Jul 6, 2021
Merged

Scheduling move checks to find best index #227

merged 2 commits into from
Jul 6, 2021

Conversation

fonsecadeline
Copy link
Contributor

@fonsecadeline fonsecadeline commented Jun 4, 2021 •
edited
Loading

@fonsecadeline fonsecadeline force-pushed the scheduling_move_checks_to_find_best_index branch from 9110fa0 to cd04bc0 Compare June 4, 2021 09:13
@fonsecadeline fonsecadeline marked this pull request as draft June 4, 2021 09:13
@fonsecadeline fonsecadeline force-pushed the scheduling_move_checks_to_find_best_index branch 3 times, most recently from 514f260 to a6feecb Compare June 8, 2021 14:24
@fonsecadeline fonsecadeline mentioned this pull request Jun 8, 2021
Closed
1 task
@fonsecadeline fonsecadeline force-pushed the scheduling_move_checks_to_find_best_index branch from a6feecb to c122d48 Compare June 11, 2021 06:39
@fonsecadeline fonsecadeline mentioned this pull request Jun 11, 2021
Merged
1 task
@fonsecadeline fonsecadeline force-pushed the scheduling_move_checks_to_find_best_index branch from c122d48 to 615575c Compare June 11, 2021 14:03
@fonsecadeline fonsecadeline marked this pull request as ready for review June 18, 2021 08:21
@fonsecadeline fonsecadeline force-pushed the scheduling_move_checks_to_find_best_index branch from 615575c to 85c442e Compare June 23, 2021 09:07
@fonsecadeline fonsecadeline force-pushed the scheduling_move_checks_to_find_best_index branch from 85c442e to 796afb6 Compare June 28, 2021 12:52
@fonsecadeline fonsecadeline force-pushed the scheduling_move_checks_to_find_best_index branch from 796afb6 to 89071fb Compare June 28, 2021 14:49
senhalil
senhalil suggested changes Jun 29, 2021
View reviewed changes
Copy link
Contributor

@senhalil senhalil left a comment

Choose a reason for hiding this comment

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

Maybe I missed but I didn't see any unit or functionality non-regression tests against the capacity violation bug. Can we add a tests that breaks without this PR and gets fixed with it?

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
lib/heuristics/concerns/scheduling_end_phase.rb Outdated Show resolved Hide resolved
@fonsecadeline fonsecadeline force-pushed the scheduling_move_checks_to_find_best_index branch from 5b5967c to b48579a Compare June 30, 2021 09:20
@fonsecadeline fonsecadeline requested a review from senhalil June 30, 2021 11:35
@fonsecadeline
Copy link
Contributor Author

@senhalil in fact it is not possible to create a code that would fail without these edits, because checks were correctly done (repeatedly) everywhere (like in find_best_day_cost for instance).

Now this capacity check is called at only once place and we are sure we are going to call it everytime we compute cost of inserting a service in a route. It was not the case before and I generated capacity violation while working on same_point_day improve. That is why I generated this PR.

@fonsecadeline fonsecadeline force-pushed the scheduling_move_checks_to_find_best_index branch from b48579a to dbc7e01 Compare July 2, 2021 08:18
@fonsecadeline fonsecadeline mentioned this pull request Jul 2, 2021
Closed
@fonsecadeline fonsecadeline force-pushed the scheduling_move_checks_to_find_best_index branch from dbc7e01 to 5bca744 Compare July 5, 2021 09:03
@fab-girard fab-girard merged commit a50de06 into Mapotempo:dev Jul 6, 2021
@fonsecadeline fonsecadeline deleted the scheduling_move_checks_to_find_best_index branch July 6, 2021 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@senhalil senhalil senhalil approved these changes

@braktar braktar braktar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@fonsecadeline @braktar @senhalil @fab-girard