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

QuantityFormatter inconsequential format strings #1183

Open
AndreasLeeb opened this issue Jan 17, 2023 · 7 comments · May be fixed by #1206
Open

QuantityFormatter inconsequential format strings #1183

AndreasLeeb opened this issue Jan 17, 2023 · 7 comments · May be fixed by #1206
Labels

Comments

@AndreasLeeb
Copy link
Contributor

Hi,

as we were having issues with how the QuantityTypeConverter displayed small values, I've dug through the code of the QuantityFormatter and found a pecularity.

If you pass the general format string "G" (or none), FormatUntrimmed always uses 2 significant digits instead of the the number specified after G (e.g. "G5"). And there is the format string "S" that allows to specify the amount of significant digits. And, unfortunately, there is no way to always display the number in normal fixed-point notation, as values below 1e-3 are displayed in scientific notation.

So I propose to move the feature that you can specify the number of significant digits and use scientific notation below 1e-3 to the format string "G" (so "G" without a number still means "G2", and for "Gn" n significant digits are shown). And for never using scientific notation we could either use "S" (as that would become obsolete when "Gn" works) or introduce a separate letter for it. However, I favor the former option.

If you agree I would change this, so that it will be part of the v5 release.

@angularsen
Copy link
Owner

angularsen commented Jan 17, 2023

Hm, the wiki docs seem outdated: https://github.com/angularsen/UnitsNet/wiki/String-Formatting

This is the latest PR that changed this:
Add support for standard numeric format strings

In particular, this xmldoc describes the current capabilities and the wiki should be updated to reflect this.
https://github.com/angularsen/UnitsNet/pull/788/files#diff-66fe4faace86846b656488e7f6cb47d9f5c74459b9803237656684483956efcf

Back to your proposal:

  • Generally, yes, let's try to mimic .NET's standard numeric format strings
  • G2, G4 etc sounds useful, f.ex. Length.FromMeters(-1.234567890e-25).ToString("G4") should be -1,235E-25 m to match (-1.234567890e-25).ToString("G4").
  • G and S are not the same in UnitsNet, so we probably should keep both.
    • G is significant digits (G4 => 1.234).
    • S is significant digits AFTER radix (S4 => 1.2345).
  • G equals S2 so Length.FromMeters(1.23456).ToString() or ToString("G") or ToString("S2"), all produce 1.23 m. This differs from .NET standard, but we wanted the default ToString() to be nice and readable.

Or so I recall, I may be off on some details.

@AndreasLeeb
Copy link
Contributor Author

Ah yes, I overlooked that small difference between G and S. So what do you think about e.g. a P for always default floating-point notation and never scientific notation or changing it for S? Because I find it confusing that S4 says 4 significant digits AFTER radix and then 0.001 gets displayed as 1e-03 even though one would expect the 1 significant digit of the number to be behind the comma and not normalized to scientific notation. Also, .NET uses non-scientific notation down to 0.0001 and only for 1e-5 it uses scientific notation.

@angularsen
Copy link
Owner

Yes, for good or worse, we decided on a different breaking point for scientific notation that felt more easy to read.

Beyond that, I think the behavior is inline with .NET, that the breaking point to scientific notation ignores the requested precision. The difference is that we break on 1e3 instead of 1e5, which can be a separate discussion.

0.0001.ToString("G4") // 0.0001
0.00001.ToString("G4") // 1E-05
0.00001.ToString("G5") // 1E-05

Trying out a few things, it seems our implementation has some bugs:

Length.FromMeters(50.001).ToString("S1") // 50 m, expected 50.0 m
Length.FromMeters(50.001).ToString("S2") // 50 m, expected 50.00 m
Length.FromMeters(50.001).ToString("S3") // 50.001 m, OK

Length.FromMeters(50.001).ToString("G") // 50 m, OK
Length.FromMeters(50.001).ToString("G1") // 50 m, OK
Length.FromMeters(50.001).ToString("G2") // 50 m, OK 
Length.FromMeters(50.001).ToString("G4") // 50 m, OK, matches .NET rounding (4 significant digits)
Length.FromMeters(50.001).ToString("G5") // 50 m, expected 50.001 (5 significant digits)
Length.FromMeters(50.001).ToString("G6") // 50 m, expected 50.001 (6 significant digits)

Another peculiarity, G2 and S2 don't affect the precision.

@angularsen
Copy link
Owner

So what do you think about e.g. a P for always default floating-point notation and never scientific notation or changing it for S?

Yes, I think that is useful. I believe .NET uses "f" "F" format for this:
https://learn.microsoft.com/en-us/dotnet/standard/base-types/standard-numeric-format-strings

1.234567890e-25.ToString("F", CultureInfo.InvariantCulture) // 0.00
1.234567890e-25.ToString("F5", CultureInfo.InvariantCulture) // 0.00000

@angularsen
Copy link
Owner

I'd love to rectify and fix this, so if you are eager to do a pull request I'm happy to assist.
Can you create a short bullet list of the changes we want to make? And denote new vs breaking changes.

@tmilnthorp tmilnthorp linked a pull request Feb 17, 2023 that will close this issue
@tmilnthorp
Copy link
Collaborator

I created #1206 to simply stop overriding a .NET format specifier. I think we should only have "UnitsNet" custom formats, not change expected onces.

@stale
Copy link

stale bot commented Jun 18, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants