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

Fix travel times computation #237

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Fix travel times computation #237

merged 1 commit into from
Aug 18, 2021

Conversation

braktar
Copy link
Contributor

@braktar braktar commented Jun 15, 2021 •
edited
Loading

@braktar braktar marked this pull request as draft June 15, 2021 10:18
senhalil
senhalil suggested changes Jun 15, 2021
View reviewed changes
optimizer_wrapper.rb Show resolved Hide resolved
optimizer_wrapper.rb Outdated Show resolved Hide resolved
optimizer_wrapper.rb Outdated Show resolved Hide resolved
optimizer_wrapper.rb Outdated Show resolved Hide resolved
optimizer_wrapper.rb Show resolved Hide resolved
optimizer_wrapper.rb Outdated Show resolved Hide resolved
optimizer_wrapper.rb Outdated Show resolved Hide resolved
@senhalil
Copy link
Contributor

I saw too late that you put the PR in draft mode while I was reviewing :|

@braktar braktar requested a review from senhalil June 16, 2021 07:02
senhalil
senhalil reviewed Jun 16, 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.

This looks good to me. I saw that there are 3 failing real_cases tests. If the dev of this PR is correct that means we still have a bug somewhere which touches the time calculations. But we have no failing unit/functional tests. If this is the case can you please add a unit test based on an MWE?

lib/interpreters/split_clustering.rb Show resolved Hide resolved
@braktar
Copy link
Contributor Author

braktar commented Jun 16, 2021 •
edited by senhalil
Loading

This looks good to me. I saw that there are 3 failing real_cases tests. If the dev of this PR is correct that means we still have a bug somewhere which touches the time calculations. But we have no failing unit/functional tests. If this is the case can you please add a unit test based on an MWE?

@braktar braktar marked this pull request as ready for review June 18, 2021 11:41
fonsecadeline
fonsecadeline reviewed Jun 28, 2021
View reviewed changes
optimizer_wrapper.rb Show resolved Hide resolved
optimizer_wrapper.rb Show resolved Hide resolved
optimizer_wrapper.rb Show resolved Hide resolved
@fonsecadeline fonsecadeline self-requested a review June 28, 2021 13:06
@braktar braktar force-pushed the travel_timess branch 2 times, most recently from 3541fd7 to e886725 Compare July 6, 2021 12:00
fonsecadeline
fonsecadeline suggested changes Jul 6, 2021
View reviewed changes
optimizer_wrapper.rb Outdated Show resolved Hide resolved
@braktar braktar mentioned this pull request Jul 21, 2021
Closed
5 tasks
@senhalil
Copy link
Contributor

senhalil commented Aug 10, 2021 •
edited by braktar
Loading

  • @braktar I re-run all the tests because some were failing but now all are failing. I suspect it needs a manual rebase and coherence check because there is no conflict...

@braktar
Copy link
Contributor Author

braktar commented Aug 16, 2021

It seems that the tests were all failing due to a CI issue. It was expecting an image which didn't exists. It seems to be solved after the rebase

@fonsecadeline fonsecadeline merged commit 5ae6691 into Mapotempo:dev Aug 18, 2021
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

@fonsecadeline fonsecadeline fonsecadeline 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.
3 participants
@braktar @senhalil @fonsecadeline