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

NaN result is a string in JSON result #166

Closed
joker234 opened this issue Mar 24, 2021 · 5 comments · Fixed by #171
Closed

NaN result is a string in JSON result #166

joker234 opened this issue Mar 24, 2021 · 5 comments · Fixed by #171
Labels
documentation Improvements or additions to documentation
Milestone

Comments

@joker234
Copy link
Member

joker234 commented Mar 24, 2021

When you get NaN as result of an aggregation (e.g. through division by zero), the result is formatted as string ("NaN"). This is, because the JSON standard doesn't allow you to return something like NaN as number.

I see two possibilities here:

  • declare this as expected behaviour and write this behaviour in the docs
  • return null in the JSON response

Example Request:

% curl -X POST https://api.ohsome.org/v1/elements/count/ratio \
    --data-urlencode "bboxes=0,0,0.01,0.01" \
    --data-urlencode "filter=geometry:polygon and amenity=hospital" \
    --data-urlencode "filter2=geometry:polygon and beds=* and amenity=hospital"
    --data-urlencode "time=2020-08-01" \
    --data-urlencode "format=json"
{
  "attribution" : {
    "url" : "https://ohsome.org/copyrights",
    "text" : "© OpenStreetMap contributors"
  },
  "apiVersion" : "1.4.1",
  "ratioResult" : [ {
    "timestamp" : "2020-08-01T00:00:00Z",
    "value" : 0.0,
    "value2" : 0.0,
    "ratio" : "NaN"
  } ]
}

The CSV format isn't affected as most CSV descriptions (e.g. RFC 4180) don't have data types.

@joker234 joker234 added this to the 1.5 milestone Mar 24, 2021
@joker234 joker234 added this to To do in ohsome API general via automation Mar 24, 2021
@FabiKo117 FabiKo117 added the documentation Improvements or additions to documentation label Mar 25, 2021
@FabiKo117
Copy link
Contributor

Either way, I guess it's worth putting that info into the docs. My suggestion would be sticking with the "NaN" output as this is also the original produced result of the division and a standard for representing such a result.

@tyrasd
Copy link
Member

tyrasd commented Mar 25, 2021

this is also the original produced result of the division

Wait, you might have mixed something up here. Division of zero by zero is typically represented as a not-a-number value in floating point numbers, that's correct. In Java this would be for example a double variable with the value Double.NaN, typically printed as NaN. But here, the returned JSON does contain the String ("NaN") instead of a number for the cases where NaN (note the missing quotes) are "expected"!

@FabiKo117
Copy link
Contributor

No, I think I understood it correct. To me, the alternative, so returning null in the JSON value field for ratio for such a case, would be further away from the original correct result NaN, compared to "NaN".

And as I said, for whatever solution we will then go for, it's definitely worth mentioning it in a note within the docs.

@tyrasd
Copy link
Member

tyrasd commented Mar 26, 2021

No, I think I understood it correct.

Sorry for the nit-pick, but my assumption came from the fact that it's simply not correct that "NaN" is the original produced result of the division, and also not the standard for representing such a result.

further away

That's a matter of taste I would say. 😉

IMHO the biggest argument pro keeping NaN would be that it's backwards compatible to what we currently return in v1 of the API.

Mentioning the fact that "ratio" results might contain "NaN" in the docs should be quick and easy fix for this issue.

@FabiKo117
Copy link
Contributor

FabiKo117 commented Mar 29, 2021

Mentioning the fact that "ratio" results might contain "NaN" in the docs should be quick and easy fix for this issue.

👍

ohsome API general automation moved this from To do to Done Apr 1, 2021
FabiKo117 added a commit that referenced this issue Apr 1, 2021
add info regarding "NaN" ratio to docs
closes #166
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
Development

Successfully merging a pull request may close this issue.

3 participants