fix(json_render): json_render is not accurate enough for extremely sm… #6531

Rejudge-F · 2023-01-30T03:01:05Z

fix(json_render): json_render is not accurate enough for extremely small numbers. eg: fmt::format_to(std::back_inserter(buffer), FMT_COMPILE("{}"), double(0.0000000000017114087924596788)); you will get a result of 1.7114087924, this is a completely wrong result.

formatter params(width & precision chapter): https://en.cppreference.com/w/cpp/utility/format/formatter

Issue

#6513

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation)
review
adjust for comments
cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

…all numbers. eg: fmt::format_to(std::back_inserter(buffer), FMT_COMPILE({}), double(0.0000000000017114087924596788)); you will get a result of 1.7114087924, this is a completely wrong result.

danpat · 2023-01-30T05:28:23Z

@Rejudge-F This introduces a C++20 construct, but so far, this codebase is C++17. @mjjbell @SiarheiFedartsou How do you feel about bumping the minimum C++ version needed?

Rejudge-F · 2023-01-30T06:10:34Z

@Rejudge-F This introduces a C++20 construct, but so far, this codebase is C++17. @mjjbell @SiarheiFedartsou How do you feel about bumping the minimum C++ version needed?

I don't think we need to bump the minimum C++ version needed, because the format_to function used by osrm is supported by the third-party fmt-9.1.0(code's here: third_party/fmt-9.1.0 ).

The above reference to the cpp reference is for the convenience of explaining the format_to parameters

SiarheiFedartsou · 2023-01-30T06:24:38Z

@danpat indeed, it seems @Rejudge-F is right and we can support it with C++17(on another hand std::format from C++20 would allow us to get rid from dependency). But in general I don't see a lot of reasons to not use C++20 at the moment - we don't have obligations to maintain compatibility with old compilers.

include/util/json_renderer.hpp

SiarheiFedartsou · 2023-01-30T14:59:53Z

unit_tests/util/json_render.cpp

    BOOST_CHECK_EQUAL(str, "42");
 }

+BOOST_AUTO_TEST_CASE(test_json_issue_6531) {


@Rejudge-F Cool that you found this g suffix! Thanks! May I ask you to add 3-4 test cases more in order to make sure that the rest of the things work as expected? e.g. 42.1, 42.0, 0.12345678912345?

Let me know if you don't have time on it and I will try to do that on my own.

I will do it.

Rejudge-F · 2023-01-31T02:17:40Z

@SiarheiFedartsou About this issue test cases has added.

DennisOSRM · 2023-01-31T06:58:49Z

Nice change.

…F/osrm-backend into bugfix-error_formatting_doubles

SiarheiFedartsou · 2023-02-01T15:25:06Z

Hey @Rejudge-F
I just re-run failing job - it seems there was some one-time issue, but it still fails due to precision issues in some of tests:

2023-02-01T13:03:49.2459832Z 1) Scenario: �[1mSpeed annotations should handle zero segments�[22m - �[90mfeatures/testbot/annotations.feature:96�[39m
2023-02-01T13:03:49.2460578Z    Step: �[1mWhen I route I should get�[22m - �[90mfeatures/testbot/annotations.feature:116�[39m
2023-02-01T13:03:49.2461198Z    Step Definition: �[90mfeatures/step_definitions/routing.js:4�[39m
2023-02-01T13:03:49.2461529Z    Message:
2023-02-01T13:03:49.2463027Z      �[31mTables were not identical:
2023-02-01T13:03:49.2463708Z      | from | to | route   | a:speed |     a:distance                  | a:duration | a:nodes |
2023-02-01T13:03:49.2464328Z      | a    | c  | abc,abc | 10:10   | (-) 249.987618946:299.962882039 | 25:30      | 1:2:3   |
2023-02-01T13:03:49.2464882Z      | a    | c  | abc,abc | 10:10   | (+) 249.9876189:299.962882      | 25:30      | 1:2:3   |�[39m

Will you be able to update it too please?

Rejudge-F · 2023-02-01T16:01:29Z

Hey @Rejudge-F

I just re-run failing job - it seems there was some one-time issue, but it still fails due to precision issues in some of tests:


2023-02-01T13:03:49.2459832Z 1) Scenario: �[1mSpeed annotations should handle zero segments�[22m - �[90mfeatures/testbot/annotations.feature:96�[39m

2023-02-01T13:03:49.2460578Z    Step: �[1mWhen I route I should get�[22m - �[90mfeatures/testbot/annotations.feature:116�[39m

2023-02-01T13:03:49.2461198Z    Step Definition: �[90mfeatures/step_definitions/routing.js:4�[39m

2023-02-01T13:03:49.2461529Z    Message:

2023-02-01T13:03:49.2463027Z      �[31mTables were not identical:

2023-02-01T13:03:49.2463708Z      | from | to | route   | a:speed |     a:distance                  | a:duration | a:nodes |

2023-02-01T13:03:49.2464328Z      | a    | c  | abc,abc | 10:10   | (-) 249.987618946:299.962882039 | 25:30      | 1:2:3   |

2023-02-01T13:03:49.2464882Z      | a    | c  | abc,abc | 10:10   | (+) 249.9876189:299.962882      | 25:30      | 1:2:3   |�[39m

Will you be able to update it too please?

Willing to

Rejudge-F · 2023-02-02T05:40:52Z

I have fixed all cucumber issue.
detail:

Failures:

1) Scenario: Car - DC North capitol situation, two on one off - features/car/conditional_restrictions.feature:499
   Step: When I route I should get - features/car/conditional_restrictions.feature:539
   Step Definition: features/step_definitions/routing.js:4
   Message:
     Tables were not identical:
     | from | to |     route                                      |     turns                                   |
     | a    | e  | (-) cap south,florida nw,florida nw,florida ne | (-) depart,turn right,continue uturn,arrive |
     | a    | e  | (+) cap south,florida ne,florida ne            | (+) depart,turn left,arrive                 |
     | f    | d  |     cap north,florida nw,florida nw            |     depart,turn left,arrive                 |
     | e    | c  | (-) florida ne,florida nw,cap south,cap south  | (-) depart,continue uturn,turn right,arrive |
     | e    | c  | (+) florida ne,cap south,cap south             | (+) depart,turn left,arrive                 |

2) Scenario: Car - DC North capitol situation, one on two off - features/car/conditional_restrictions.feature:546
   Step: When I route I should get - features/car/conditional_restrictions.feature:586
   Step Definition: features/step_definitions/routing.js:4
   Message:
     Tables were not identical:
     | from | to |     route                                      |     turns                                         |
     | a    | e  |     cap south,florida ne,florida ne            |     depart,turn left,arrive                       |
     | f    | d  | (-) cap north,florida ne,florida ne,florida nw | (-) depart,turn sharp right,continue uturn,arrive |
     | f    | d  | (+) cap north,florida nw,florida nw            | (+) depart,turn left,arrive                       |
     | e    | c  |     florida ne,cap south,cap south             |     depart,turn left,arrive                       |

3) Scenario: Speed annotations should handle zero segments - features/testbot/annotations.feature:96
   Step: When I route I should get - features/testbot/annotations.feature:116
   Step Definition: features/step_definitions/routing.js:4
   Message:
     Tables were not identical:
     | from | to | route   | a:speed |     a:distance                  | a:duration | a:nodes |
     | a    | c  | abc,abc | 10:10   | (-) 249.987618946:299.962882039 | 25:30      | 1:2:3   |
     | a    | c  | abc,abc | 10:10   | (+) 249.9876189:299.962882      | 25:30      | 1:2:3   |

4) Scenario: Regression test - add source phantom properly (two phantoms on one edge) - features/testbot/matching.feature:792
   Step: When I match I should get - features/testbot/matching.feature:811
   Step Definition: features/step_definitions/matching.js:7
   Message:
     Tables were not identical:
     | trace | geometry             |     a:distance   | a:duration | a:weight | duration |
     | 2345  | 1.00018,1,1.000314,1 | (-) 14.914666491 | 1.4        | 1.4      | 1.4      |
     | 2345  | 1.00018,1,1.000314,1 | (+) 14.91466649  | 1.4        | 1.4      | 1.4      |
     | 4321  | 1.00027,1,1.000135,1 | (-) 15.025969972 | 1.5        | 1.5      | 1.5      |
     | 4321  | 1.00027,1,1.000135,1 | (+) 15.02596997  | 1.5        | 1.5      | 1.5      |

5) Scenario: Shortest via path with continuation, multiple waypoint snapping candidates - features/testbot/snap_intersection.feature:529
   Step: When I route I should get - features/testbot/snap_intersection.feature:566
   Step Definition: features/step_definitions/routing.js:4
   Message:
     Tables were not identical:
     | waypoints | route             |     time           |     weight        |
     | a,f       | ad,df,df          |     40s            |     40            |
     | f,k       | fj,kj,kj          |     65.6s          |     65.6          |
     | a,f,k     | ac,cf,cf,fj,kj,kj |     132.8s         |     132.8         |
     | k,f       | ik,fi,fi          |     54.3s          |     54.3          |
     | f,a       | ef,ae,ae          |     66.6s          |     66.6          |
     | k,f,a     | kj,fj,fj,ef,ae,ae | (-) 141.399999999s | (-) 141.399999999 |
     | k,f,a     | kj,fj,fj,ef,ae,ae | (+) 141.4s         | (+) 141.4         |

6) Scenario: Shortest via path with uturns, multiple waypoint snapping candidates - features/testbot/snap_intersection.feature:582
   Step: When I route I should get - features/testbot/snap_intersection.feature:622
   Step Definition: features/step_definitions/routing.js:4
   Message:
     Tables were not identical:
     | waypoints | route             |     time           |     weight        |
     | a,f       | ad,df,df          |     40s            |     40            |
     | f,k       | fj,kj,kj          |     65.6s          |     65.6          |
     | a,f,k     | ad,df,df,fj,kj,kj |     105.6s         |     105.6         |
     | k,f       | ik,fi,fi          |     54.3s          |     54.3          |
     | f,a       | ef,ae,ae          |     66.6s          |     66.6          |
     | k,f,a     | ik,fi,fi,ef,ae,ae | (-) 120.899999999s | (-) 120.899999999 |
     | k,f,a     | ik,fi,fi,ef,ae,ae | (+) 120.9s         | (+) 120.9         |

7) Scenario: Start and target on the same and adjacent edge - features/testbot/weight.feature:39
   Step: When I route I should get - features/testbot/weight.feature:54
   Step Definition: features/step_definitions/routing.js:4
   Message:
     Tables were not identical:
     | waypoints | route   | distances | weights | times   |     a:distance                | a:duration | a:weight | a:speed |
     | s,t       | abc,abc | 20m,0m    | 2,0     | 2s,0s   | (-) 20.034626629              | 2          | 2        | 10      |
     | s,t       | abc,abc | 20m,0m    | 2,0     | 2s,0s   | (+) 20.03462663               | 2          | 2        | 10      |
     | t,s       | abc,abc | 20m,0m    | 2,0     | 2s,0s   | (-) 20.034626629              | 2          | 2        | 10      |
     | t,s       | abc,abc | 20m,0m    | 2,0     | 2s,0s   | (+) 20.03462663               | 2          | 2        | 10      |
     | s,e       | abc,abc | 40m,0m    | 3.9,0   | 3.9s,0s | (-) 29.940636463:10.017313314 | 3:0.9      | 3:0.9    | 10:11.1 |
     | s,e       | abc,abc | 40m,0m    | 3.9,0   | 3.9s,0s | (+) 29.94063646:10.01731331   | 3:0.9      | 3:0.9    | 10:11.1 |
     | e,s       | abc,abc | 40m,0m    | 3.9,0   | 3.9s,0s | (-) 10.017313314:29.940636463 | 0.9:3      | 0.9:3    | 11.1:10 |
     | e,s       | abc,abc | 40m,0m    | 3.9,0   | 3.9s,0s | (+) 10.01731331:29.94063646   | 0.9:3      | 0.9:3    | 11.1:10 |

1351 scenarios (7 failed, 1344 passed)
7246 steps (7 failed, 1 skipped, 7238 passed)
5m12.590s

SiarheiFedartsou · 2023-02-02T09:07:52Z

features/car/conditional_restrictions.feature

        When I route I should get
            | from | to | route                                      | turns                                       |
-            | a    | e  | cap south,florida nw,florida nw,florida ne | depart,turn right,continue uturn,arrive     |
+            | a    | e  | cap south,florida ne,florida ne | depart,turn left,arrive     |


I am curious how it is possible that it was changed due to changes in JSON rendering? 🤔

I guess it is due to some caches on CI because you have new failure which says you have "original" values here
https://pipelines.actions.githubusercontent.com/serviceHosts/5f8b3a21-777f-4ccd-83b2-9ebc854aa1e0/_apis/pipelines/1/runs/1148/signedlogcontent/31?urlExpires=2023-02-02T09%3A15%3A35.9673538Z&urlSigningMethod=HMACV1&urlSignature=k5UH1girtk3krsR2TdceIF1EsrnxhL2RtXruekV8s%2BU%3D

This case is torturing me non-stop hahahaha, do you have a good solution?

Yes, I think I can try dropping CI caches. Sorry, I am a bit busy right now, but will try to do that this evening(not sure which time zone you are, it will be in ~8-10 hours).

Then this issue will be handed over to you, and I will no longer submit relevant code. I'm in GMT+8

SiarheiFedartsou · 2023-02-02T17:58:06Z

@Rejudge-F many thanks for this contribution!

DennisOSRM · 2023-02-02T18:30:37Z

Very nice contribution. Tyvm

Project-OSRM#6531)

fix(json_render): json_render is not accurate enough for extremely sm…

b372611

…all numbers. eg: fmt::format_to(std::back_inserter(buffer), FMT_COMPILE({}), double(0.0000000000017114087924596788)); you will get a result of 1.7114087924, this is a completely wrong result.

SiarheiFedartsou reviewed Jan 30, 2023

View reviewed changes

include/util/json_renderer.hpp Outdated Show resolved Hide resolved

fix(json_render): add json render unittest & optimized Number formatter.

16c5a64

Rejudge-F force-pushed the bugfix-error_formatting_doubles branch from 7e0d760 to 16c5a64 Compare January 30, 2023 11:38

SiarheiFedartsou reviewed Jan 30, 2023

View reviewed changes

fix(json_render): add some unit test case for test_json_issue_6531

df926c5

Rejudge-F requested a review from SiarheiFedartsou January 31, 2023 07:37

SiarheiFedartsou and others added 4 commits January 31, 2023 14:56

Fix format

a32dfed

Add boost/assert.hpp include

3585c1e

fix(json_render): fix test case precision.

584382d

Merge branch 'bugfix-error_formatting_doubles' of github.com:Rejudge-…

fa28b39

…F/osrm-backend into bugfix-error_formatting_doubles

fix(json_render): fix cucumber test case precision issue.

8a2eb41

fix conditional_restrictions.feature

11ac58a

SiarheiFedartsou reviewed Feb 2, 2023

View reviewed changes

Try to fix issue with CI cache

6c8131a

SiarheiFedartsou self-requested a review February 2, 2023 17:58

SiarheiFedartsou approved these changes Feb 2, 2023

View reviewed changes

SiarheiFedartsou merged commit ebd9ab4 into Project-OSRM:master Feb 2, 2023

Rejudge-F deleted the bugfix-error_formatting_doubles branch February 3, 2023 03:08

mattwigway pushed a commit to mattwigway/osrm-backend that referenced this pull request Jul 20, 2023

fix(json_render): json_render is not accurate enough for extremely sm… (

2f6723d

Project-OSRM#6531)

fenwuyaoji mentioned this pull request Aug 16, 2024

Json_render is not accurate for osm node ID #7016

Open

imannamdari mentioned this pull request Feb 21, 2025

Fix json rendering of large osm ids #7096

Open

6 tasks

bevanjkay mentioned this pull request Apr 21, 2025

osrm-backend 6.0.0 Homebrew/homebrew-core#220847

Merged

fix(json_render): json_render is not accurate enough for extremely sm… #6531

fix(json_render): json_render is not accurate enough for extremely sm… #6531

Uh oh!

Conversation

Rejudge-F commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Tasklist

Requirements / Relations

Uh oh!

danpat commented Jan 30, 2023

Uh oh!

Rejudge-F commented Jan 30, 2023

Uh oh!

SiarheiFedartsou commented Jan 30, 2023

Uh oh!

Uh oh!

SiarheiFedartsou Jan 30, 2023

Choose a reason for hiding this comment

Uh oh!

Rejudge-F Jan 31, 2023

Choose a reason for hiding this comment

Uh oh!

Rejudge-F commented Jan 31, 2023

Uh oh!

DennisOSRM commented Jan 31, 2023

Uh oh!

SiarheiFedartsou commented Feb 1, 2023

Uh oh!

Rejudge-F commented Feb 1, 2023

Uh oh!

Rejudge-F commented Feb 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SiarheiFedartsou Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

SiarheiFedartsou Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

Rejudge-F Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

SiarheiFedartsou Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

Rejudge-F Feb 2, 2023

Choose a reason for hiding this comment

Uh oh!

SiarheiFedartsou commented Feb 2, 2023

Uh oh!

DennisOSRM commented Feb 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rejudge-F commented Jan 30, 2023 •

edited

Loading

Rejudge-F commented Feb 2, 2023 •

edited

Loading