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

Cockpit: limit number of shown decimal places when formatting numbers #2182

Closed
wants to merge 1 commit into from

Conversation

@dperpeet
Copy link
Member

commented Apr 20, 2015

See testcase
555.98765432, "556.0 bps"
is this what we want?

For a "clean" solution we'd have to check if the rounded result is an integer (or even === factor) and change things accordingly.

I believe this output is ok, since it indicates that higher precision than integer is at play and that the result may have been affected by rounding.

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2015

I think this makes sense, yes.

@dperpeet dperpeet force-pushed the dperpeet:network_traffic branch from adece1f to 5592d6d Apr 20, 2015

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2015

result of irc discussion: ".0" at the end of a number is redundant.

@dperpeet dperpeet force-pushed the dperpeet:network_traffic branch from 5592d6d to a402955 Apr 20, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2015

result of irc discussion: ".0" at the end of a number is redundant.

It is only redundant when the result is exactly X of the factor. If it has been rounded to X, then we should use X.0

An examples:

  • 1024 bytes -> 1 KB
  • 1025 bytes -> 1.0 KB

Going with the above ... since bps is a speed, and unless the speed is exactly an integer bits per second, the latter should be used.

Even if @andreasn insists that we shouldn't do this for "bps", that shouldn't change how we do this for other units.

ret = [number.toFixed(1), suffix];
else
ret = [number.toFixed(1)];

This comment has been minimized.

Copy link
@stefwalter

stefwalter Apr 21, 2015

Contributor

This changes the API. This API is meant to return two values unless !separate. What's the logic behind this change. This change is also not documented.

This comment has been minimized.

Copy link
@dperpeet

dperpeet Apr 21, 2015

Author Member

This changes the API. This API is meant to return two values unless !separate. What's the logic behind this change. This change is also not documented.

This is not an API change, just moving inside the function. See current master

This comment has been minimized.

Copy link
@stefwalter

stefwalter Apr 21, 2015

Contributor

Aha, makes sense.

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2015

It is only redundant when the result is exactly X of the factor. If it has been rounded to X, then we should use X.0

I was of your opinion, but changed my mind after discussing with Andreas and Marius. After all, reading the trailing .0 seems redundant. Marius pointed out that the distinction we make may not be clear to the user anyway.

@andreasn How should we proceed on this?

@mvollmer mvollmer referenced this pull request Apr 21, 2015

@dperpeet dperpeet force-pushed the dperpeet:network_traffic branch from a402955 to 672b2a3 Apr 21, 2015

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2015

I think the best choice is to show natural numbers as such and everything else in fixed point notation, even with a trailing zero. For this it doesn't matter if the natural number resulted from division or not.

Example: If the result of the division is a natural number - at least within double precision - we should consider it as such (e.g. 2048 / 1024 is 2, not 2.0).

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2015

As an alternative we could say that we drop the decimal place completely for values >= 10.
This is equivalent to saying we want at least 2 significant digits: 5.5, 9.8, 11

@andreasn

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2015

My biggest concern right now is the numbers on the left of the graphs in #2020
In general elsewhere, like on the top right of the same graphs I would be more OK with seeing a 3.0 or so.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2015

My biggest concern right now is the numbers on the left of the graphs in #2020

First of all, I if it's exactly N units, we should display N and not N.0 ... The latter should be displayed when we discard information during rounding.

And as noted in #2020, I think that should be fixed in the graph itself to display Y axis grid lines intelligently at N units, similar to how we display lines intelligently on the X axis intelligently (on minutes).

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2015

First of all, I if it's exactly N units, we should display N and not N.0 ... The latter should be displayed when we discard information during rounding.

The current implementation ignores information lost due to js internal number representation. I believe this to be acceptable and even necessary when dealing with large integers - since js only knows 64 bit fp.

@mvollmer

This comment has been minimized.

Copy link
Member

commented Apr 21, 2015

As an alternative we could say that we drop the decimal place completely for values >= 10.
This is equivalent to saying we want at least 2 significant digits: 5.5, 9.8, 11

JavaScript has X.toPrecision(2), which produces 5.5 and 11, but it might also produce 0.30 instead of 0.3, i.e., it doesn't count a leading "0.".

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2015

JavaScript has X.toPrecision(2), which produces 5.5 and 11, but it might also produce 0.30 instead of 0.3, i.e., it doesn't count a leading "0.".

even worse. toPrecision can produce scientific notation for numbers larger than the precision you pass (e.g. 937 could become 9.4e+2)

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2015

@andreasn @stefwalter how should we proceed?
I would be ok leaving this as it is in the general case.
Options I could add:

  • flag to remove trailing .0 if present
  • [change default to / add flag to] show decimal place only for values <10
@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2015

I'll work on this. We should detect exact multiples of units, and drop the .0 in that case.

@stefwalter stefwalter self-assigned this Apr 22, 2015

@dperpeet

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2015

@stefwalter exact multiples will be detected and are without .0 - all natural numbers are converted using standard number-to-string, not fixed notation:
https://github.com/cockpit-project/cockpit/pull/2182/files#diff-34cbdb779d20a4a75591fcba50bd25d4R2182

@dperpeet dperpeet closed this in b10b07e Apr 22, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2015

You're right. I added more tests, and merged.

@dperpeet dperpeet deleted the dperpeet:network_traffic branch Jun 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.