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

Limit decimal places in JSON output of averages #161

Merged
merged 3 commits into from
Jul 29, 2020
Merged

Limit decimal places in JSON output of averages #161

merged 3 commits into from
Jul 29, 2020

Conversation

graeme-a-stewart
Copy link
Member

As nlohmann::json always attempts to maintain full precision
when converting from a C++ float/double to a JSON number
it prints way too many decimal places in many cases.

Use std::modf to split the double precision value into integer
and fractional pieces and then truncate the DPs if needed.

Note that it's quite tricky to do this, so the fractional piece is
stored as a volatile to prevent the 'int(value*N)/double(N)' from
being optimised away. Note also that this is quite an expensive
operation, but in the context of prmon we can afford this at the
typical monitoring cadence.

Closes #158

As nlohmann::json always attempts to maintain full precision
when converting from a C++ float/double to a JSON number
it prints way too many decimal places in many cases.

Use std::modf to split the double precision value into integer
and fractional pieces and then truncate the DPs if needed.

Note that it's quite tricky to do this, so the fractional piece is
stored as a volatile to prevent the 'int(value*N)/double(N)' from
being optimised away. Note also that this is quite an expensive
operation, but in the context of prmon we can afford this at the
typical monitoring cadence.
@graeme-a-stewart graeme-a-stewart requested a review from amete July 28, 2020 10:10
@graeme-a-stewart graeme-a-stewart added the enhancement New feature or request label Jul 28, 2020
Copy link
Collaborator

@amete amete left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this @graeme-a-stewart. Maybe this is splitting hairs but I don't think we're strictly limiting our measurement to a per-mille level accuracy here, no? What we're instead doing is that anything larger than 1k will be converted to an integer, while anything less than 1k will have three decimal places. E.g. 123456.1236 will be 123456 and 123.1236 will be 123.123, where the latter also suffers a bias from the lack of rounding (which would affect the first, too, of course). They both imply precision that is better than per-mille and we're not doing any rounding correction etc.

Having said that, I think what we have is good enough for our purposes. It's just that we should perhaps advertise this a bit clearer, unless I'm missing something.

Please let me know, many thanks.

@graeme-a-stewart
Copy link
Member Author

Hi @amete - yes, you're right, it's printing a rounded integer for > 1000 and truncating to 3DP for <1000.

I wanted to avoid complexities like calculating log10 to get more exact significant figure rounding.

If you also agree this is good enough I will update the description to be more accurate of the actual method.

@amete
Copy link
Collaborator

amete commented Jul 29, 2020

If you also agree this is good enough I will update the description to be more accurate of the actual method.

I completely agree this is good enough for all practical purposes. Updating the description sounds like a good idea, just to make sure there are no misunderstandings. Many thanks.

Copy link
Collaborator

@amete amete left a comment

Choose a reason for hiding this comment

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

LGTM

@amete amete merged commit 298aaaa into master Jul 29, 2020
@amete amete deleted the rounding branch July 29, 2020 13:46
@amete amete mentioned this pull request Sep 7, 2020
@graeme-a-stewart graeme-a-stewart added this to the v2.1 milestone Sep 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Round average values to more resonable number of DPs
2 participants