Skip to content

Fix floating point formatting, #799 #801

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

Merged
merged 3 commits into from
May 2, 2020

Conversation

sparkprime
Copy link
Collaborator

No description provided.

@sparkprime
Copy link
Collaborator Author

Took me a while to realise that render_int would actually take a floating point number and internally do the right thing with the sign

@@ -535,7 +535,7 @@ limitations under the License.
local whole = std.floor(n_);
local dot_size = if prec == 0 && !ensure_pt then 0 else 1;
local zp = zero_pad - prec - dot_size;
local str = render_int(std.sign(n__) * whole, zp, 0, blank, sign, 10, '');
local str = render_int(n__, zp, 0, blank, sign, 10, '');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you modify the comment for render_int explaining what it does with non-whole numbers? This behavior is counter-intuitive, so it's better to have it spelled out explicitly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Have attempted to do it less hackily now

@@ -189,6 +189,7 @@ std.assertEqual(std.format('%-12.4f', [910.3]), '910.3000 ') &&
std.assertEqual(std.format('%#.0f', [910.3]), '910.') &&
std.assertEqual(std.format('%#.0f', [910]), '910.') &&
std.assertEqual(std.format('%.3f', [1000000001]), '1000000001.000') &&
std.assertEqual(std.format('%f', [-0.1]), '-0.100000') &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also add `std.format('%d', [-0.1])'. The result will be '-0', I think. Not sure if that's ok.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's not OK because it's different to what Python does.

Copy link
Collaborator

@sbarzowski sbarzowski left a comment

Choose a reason for hiding this comment

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

Thanks for dealing with this bug!

// min_chars must be a whole number >= 0
// It is the field width, i.e. std.length() of the result should be >= min_chars
// min_digits must be a whole number >= 0. It's the number of zeroes to pad with.
// blank must be a boolean, if true a postive number has a ' ' in place of the '-'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The detailed explanation is good, but I think these lines are confusing.

  • "blank must be a boolean, if true a postive number has a ' ' in place of the '-'." <- It's weird to say that a positive number has something in place of '-'. A positive number has no '-'. Perhaps it would be better to say that it adds an additional ' ' in front of the number, so that it is aligned with negative numbers with the same number of digits.
  • plus must be a boolean, if true a '+' is used in place of the '-'. <- Same, but even more confusing. Naively, it sounds like we would format -5 as "+5" when plus is true.

@@ -53,6 +53,27 @@ std.assertEqual(std.format('thing-%-5.3d', [10.3]), 'thing-010 ') &&
std.assertEqual(std.format('thing-%#-5.3d', [10.3]), 'thing-010 ') &&
std.assertEqual(std.format('thing-%#-5.3i', [10.3]), 'thing-010 ') &&
std.assertEqual(std.format('thing-%#-5.3u', [10.3]), 'thing-010 ') &&
std.assertEqual(std.format('thing-%5.3d', [-10.3]), 'thing- -010') &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice to have all these cases covered!

@sbarzowski sbarzowski merged commit 29fd570 into google:master May 2, 2020
@sparkprime sparkprime deleted the fix_float_fmt branch October 26, 2022 13:47
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 this pull request may close these issues.

3 participants