Skip to content

seq: fix default float precision (%f); fix 0 scientific printing #7384

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 4 commits into from
Mar 14, 2025

Conversation

drinkcat
Copy link
Contributor

@drinkcat drinkcat commented Mar 3, 2025

The fact that printf and seq path are totally different is, TBH, a bit confusing...

printf uses precision 6 because of this subtle line in spec.rs:Spec::write (Self::Float case):

let precision = resolve_asterisk(*precision, &mut args).unwrap_or(6);

2 small additional changes (I can split to 2 other PR if needed, they felt small enough to just tack on this one):

  • While adding one of the seq test, noticed that printing of 0 with capitalized %E was wrong, fix that as well.
  • And noticed 2 seq tests that were disabled that can be enabled.

seq: Enable test_auto_precision and test_undefined

Those tests appear to have been fixed, enable them.

seq: Add tests for default float formats

Add tests for some of the default float formats (%f, %g, %E), mostly
to check that the default precision is correctly set to 6 digits.

uucore: format: Fix default Float precision in try_from_spec

The default precision is 6, no matter the format. This applies
to all float formats, not just "%g" (aka FloatVariant::Shortest).

Fixes #7361.

uucore: format: Fix capitalization of 0 in scientific formating

0.0E+00 was not capitalized properly when using %E format.

Fixes #7382.

Test: cargo test --package uucore --all-features float
Test: cargo run printf "%E\n" 0 => 0.000000E+00

Copy link

github-actions bot commented Mar 3, 2025

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/stdbuf (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

Copy link

github-actions bot commented Mar 5, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@drinkcat
Copy link
Contributor Author

drinkcat commented Mar 7, 2025

Retrigger CI with a force push, but I think the error count difference came from tests/timeout/timeout.

@sylvestre sylvestre requested a review from Copilot March 7, 2025 09:19
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR addresses inconsistencies in float precision handling and scientific notation formatting, and enables additional tests for verifying these behaviors.

  • Update default float precision to 6 across all variants.
  • Fix the capitalization of 0 in scientific notation and remove redundant code.
  • Enable and add tests for default formatting for %f, %g, and %E formats.

Reviewed Changes

File Description
src/uucore/src/lib/features/format/num_format.rs Adjusts default precision and fixes scientific formatting for both zero and non-zero.
tests/by-util/test_seq.rs Enables and adds tests to verify default precision and formatting behavior.

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

Copy link

github-actions bot commented Mar 7, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

Copy link

github-actions bot commented Mar 7, 2025

GNU testsuite comparison:

Skip an intermittent issue tests/misc/stdbuf (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/timeout/timeout (passes in this run but fails in the 'main' branch)

Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@@ -293,13 +293,7 @@ impl Formatter for Float {

let precision = match precision {
Some(CanAsterisk::Fixed(x)) => x,
None => {
if matches!(variant, FloatVariant::Shortest) {
Copy link
Member

Choose a reason for hiding this comment

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

Hmm good catch. I was wondering how this came to be and it seems to be this: 4c5326f

Looks like that commit only looked at %g and check the other formats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah... Also... Default precision for %a is not 6, but... the hexadecimal printing function right now does not support any sort of precision setting (e.g. %.6a still prints all digits...).

I'm... slowly trying to untangle this, maybe better to fix #7364 first. Still filed #7429 to remember.

@drinkcat
Copy link
Contributor Author

Just did another rebase. Anything blocking this? (I have other changes on top of this that are starting to pile up...)

Copy link

GNU testsuite comparison:

GNU test failed: tests/misc/tee. tests/misc/tee is passing on 'main'. Maybe you have to rebase?

0.0E+00 was not capitalized properly when using `%E` format.

Fixes uutils#7382.

Test: cargo test --package uucore --all-features float
Test: cargo run printf "%E\n" 0 => 0.000000E+00
The default precision is 6, no matter the format. This applies
to all float formats, not just "%g" (aka FloatVariant::Shortest).

Fixes uutils#7361.
Add tests for some of the default float formats (%f, %g, %E), mostly
to check that the default precision is correctly set to 6 digits.
Those tests appear to have been fixed, enable them.
Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/timeout/timeout (fails in this run but passes in the 'main' branch)

@RenjiSann
Copy link
Collaborator

Thanks !

@RenjiSann RenjiSann merged commit c0a1179 into uutils:main Mar 14, 2025
66 checks passed
@drinkcat drinkcat deleted the seq-float branch March 14, 2025 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

printf: %E format for 0.0 is not capitalized seq: default float output format %f incorrect
3 participants