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

front: fix travel time >1h in OSRD → NGE conversion #9035

Merged
merged 1 commit into from
Oct 1, 2024
Merged

front: fix travel time >1h in OSRD → NGE conversion #9035

merged 1 commit into from
Oct 1, 2024

Conversation

emersion
Copy link
Member

Multiple issues here:

  • Source or target "time" may be zero or null, zero means it's set but top of the hour, null means unset/unknown. We still want to compute the travel time if it's zero.
  • Travel "time" field doesn't need to be computed modulo 1h, because it's a duration, not an absolute time.
  • Travel "consecutiveTime" field needs to be populated as well.

Closes: #8862

@emersion emersion requested a review from a team as a code owner September 27, 2024 08:56
louisgreiner
louisgreiner approved these changes Sep 27, 2024
View reviewed changes
Copy link
Contributor

@louisgreiner louisgreiner left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested and it's working very well, thanks :)

For the code part, I still have some warning about linting

@emersion emersion force-pushed the emr/fix-nge-1h-travel branch 2 times, most recently from b28a16d to ccbf09a Compare September 27, 2024 09:26
@emersion
Copy link
Member Author

For the code part, I still have some warning about linting

Apologies, should be fixed now!

@emersion
Copy link
Member Author

Small cleanup to get rid of unnecessary null assertions after discussing with @sim51.

@sim51
Copy link
Contributor

sim51 commented Sep 27, 2024

good for me !

SharglutDev
SharglutDev approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@SharglutDev SharglutDev left a comment

Choose a reason for hiding this comment

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

Lgtm and tested

@emersion
front: fix travel time >1h in OSRD → NGE conversion
4bc0dff 
Multiple issues here:

- Source or target "time" may be zero or null, zero means it's
  set but top of the hour, null means unset/unknown. We still want
  to compute the travel time if it's zero.
- Travel "time" field doesn't need to be computed modulo 1h, because
  it's a duration, not an absolute time.
- Travel "consecutiveTime" field needs to be populated as well.

Signed-off-by: Simon Ser <contact@emersion.fr>
Closes: #8862
@emersion emersion added this pull request to the merge queue Oct 1, 2024
Merged via the queue into dev with commit 8b4265f Oct 1, 2024
24 checks passed
@emersion emersion deleted the emr/fix-nge-1h-travel branch October 1, 2024 09:31
@louisgreiner louisgreiner mentioned this pull request Oct 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SharglutDev SharglutDev SharglutDev approved these changes

@louisgreiner louisgreiner louisgreiner approved these changes

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

Travel times (consecutiveTime) not set properly when > 60'
4 participants
@emersion @sim51 @louisgreiner @SharglutDev