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

Support the flag ' for thousand separator in Printf, issue #29077 #42145

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

ki-chi
Copy link

@ki-chi ki-chi commented Sep 7, 2021

Closes #29077.
The flag ' is valid only for the decimal notation, i.e., the following formats: %d, %u, %i, %f, %F, %g, and %G.

julia> using Printf

julia> @sprintf "%'d" 1000000
"1,000,000"

julia> @sprintf "%'.2f" 1000000
"1,000,000.00"

julia> @sprintf "%-'10d" 1000
"1,000     "

julia> @sprintf "%'10d" 1000
"     1,000"

ki-chi and others added 5 commits September 7, 2021 03:09
Add tests

Update the struct Spec

Extend plength() for thousand sep.

Update Base.string for Spec including apostrophe

Supports integer output

Fix wrong test

Add test for printing float as %'d

Reset the way to insert the thousand operator

Add `insertsep` function

Add tests for the combinations of the thousand separator and others

Implement the thousand operator option for %d, %u, and %i
Add tests for thousand separators for floats

Implement thousand separators for floats: %f, %F, %g, and %G

Fix `countthousandsep` for proper rounding

Add tests for BigFloat with thousand separators

Fix impl for BigFloat with thousand separators

Add tests for BigFloat with zero precision and a thousand separator

WIP: for scientific notation when %'g

Add function `findonesplace`
@KristofferC KristofferC requested a review from quinnj August 31, 2022 13:58
@KristofferC
Copy link
Member

@quinnj, maybe you could take a look at this?

stdlib/Printf/src/Printf.jl Outdated Show resolved Hide resolved
stdlib/Printf/src/Printf.jl Outdated Show resolved Hide resolved
stdlib/Printf/src/Printf.jl Outdated Show resolved Hide resolved
Copy link
Member

@quinnj quinnj left a comment

Choose a reason for hiding this comment

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

This overall looks like useful functionality. I've left a few comments about improving the performance, which we want to be sensitive too. i.e. for formats not using the apostrophe, we'd ideally have zero impact from this net-new code. I think the BigInt trunc is the only really tricky part; we need to find another, more efficient way to get that information.

@ki-chi
Copy link
Author

ki-chi commented Oct 8, 2022

@quinnj Thanks for the review. I will try to fix the code, and you are right about the BigInt issue; I need to think about it more carefully.

@nathanrboyer
Copy link
Contributor

Now that #40105 is released in Julia 1.10, I want this functionality even more (to eliminate my need for the Formatting.jl package). Maybe @johanmon, having worked on this function recently, has some ideas to avoid the performance issues above?

@GregPlowman
Copy link
Contributor

It seems this PR has stalled because of some performance concerns.

I think the BigInt trunc is the only really tricky part; we need to find another, more efficient way to get that information.

Would something like the following work as an alternative to the BigInt version?

function countthousandsep(f::Spec{T}, x) where {T <: Union{Ints, Floats}}
    (isnan(x) || isinf(x) || base(T) != 10) && return 0
    x < 0 && return countthousandsep(f, -x)
    x2 = round(x, digits=f.precision)
    x2 < 1000 && return 0
    numdig = log10(x2)
    numsep = Int(div(numdig, 3))
    return numsep
end

I think the other issues raised were using view to avoid copying, which seems simple enough.

@ki-chi, are you still willing to work on this?

@ki-chi
Copy link
Author

ki-chi commented Jan 25, 2024

@GregPlowman,

Sorry for leaving that pull request hanging. I need more time to work on this PR now. But I always welcome any suggestion to resolve the performance issue above.

@GregPlowman
Copy link
Contributor

GregPlowman commented Jan 29, 2024

@ki-chi, is this PR finished on your end? If you're happy with everything, should you the mark the requested changes as addressed?

@quinnj, would you be willing to continue your review?

@ki-chi ki-chi requested a review from quinnj February 27, 2024 07:12
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.

Support ' format flag for @printf
5 participants