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 various formats for progress rate string #228

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

Conversation

wsshin
Copy link

@wsshin wsshin commented Jan 10, 2022

This PR implements a support for various formats for the string indicating the progress rate. Currently, the string before the progress bar is in percent format (e.g., "20%"), but sometimes it is useful to know exactly how many items have been processed out of the total.

For example, if some parameter is being processed at 50 different values, but if the process is halted midway, you may want to resume the process from the halted value in order to save time. By showing the progress rate as "10/50" or "10 out of 50" instead of "20%", this PR allows the users to know exactly where the process stopped, so that s/he can modify the script to start from the halted value (in this case at the 5th value of the parameter).

Here are some example usages of the new feature:

@showprogress 1 "Computing..." FractionRateFormat() for i in 1:50  # "10/50"
    sleep(0.1)
end
@showprogress 1 "Computing..." IntegerRateFormat() for i in 1:50  # "10 of 50"
    sleep(0.1)
end

This PR also provides a framework for the users to implement their own formats for the progress rate string by extending AbstractRateFormat and implementing the corresponding rate_string() function. As an example, this PR implements

  • EmptyRateFormat, which does not pint out the progress rate
  • PercentRateFormat(digits), wheredigits indicates the number of digits below the decimal point in the percent format (e.g., 9.24% for PercenRateFormat(2))

@wsshin
Copy link
Author

wsshin commented Jan 13, 2022

If there is anything else I need to do to merge this PR, please let me know.

@martinholters
Copy link
Collaborator

Looks like this is missing tests and documentation, at least.

@wsshin
Copy link
Author

wsshin commented Jan 16, 2022

Looks like this is missing tests and documentation, at least.

Done! Let me know if anything else is needed.

@DilumAluthge
Copy link
Contributor

@martinholters @timholy What are your thoughts on this PR?

I personally am interested in using the PercentRateFormat(digits) feature, because I would like to display the percentage as e.g. 12.3% instead of just 12%.

@@ -66,6 +94,7 @@ mutable struct Progress <: AbstractProgress
tlast::Float64
printed::Bool # true if we have issued at least one status update
desc::String # prefix to the percentage, e.g. "Computing..."
rate_format::AbstractRateFormat
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we introduce a type parameter to Progress so that rate_format can be concretely typed? Otherwise, the calls to rate_string would do dynamic dispatch. But maybe that is insignificant performance-wise compared to the IO. Could you benchmark an empty loop to see whether this PR incurs any relevant performance hit?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooof. Yeah having the dynamic dispatch inside a hot loop sounds like a bad idea.

Instead of having rate_format::AbstractRateFormat, perhaps we could have a concretely-typed format_string::String?

Copy link
Author

Choose a reason for hiding this comment

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

With the master branch:

julia> p = ProgressMeter.Progress(10000000);

julia> p.rate_format  # master doesn't support rate format
ERROR: type Progress has no field rate_format
...

julia> @btime ProgressMeter.next!($p)
Progress: 100%|███████████████████████████████████████████████████████████████████████████████████████| Time: 0:00:02
  16.335 ns (0 allocations: 0 bytes)

With the present PR:

julia> p = ProgressMeter.Progress(10000000);

julia> p.rate_format  # present PR supports rate format
PercentRateFormat(0)

julia> @btime ProgressMeter.next!($p)
Progress: 100%|███████████████████████████████████████████████████████████████████████████████████████| Time: 0:00:02
  15.517 ns (0 allocations: 0 bytes)

There doesn't seem any noticeable performance degradation.

Copy link
Owner

Choose a reason for hiding this comment

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

More informative might be something like this:

julia> p = ProgressMeter.Progress(10000000; dt=0.001);

julia> @bprofile ProgressMeter.next!($p)
Progress: 100%|█████████████████████████████████████████████████████████████████████████████████| Time: 0:00:02
BenchmarkTools.Trial: 10000 samples with 1000 evaluations.
 Range (min  max):  5.342 ns  624.207 ns  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     6.455 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   8.209 ns ±  15.537 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

  ▄█▄▄▂                                                       ▁
  ██████▆▅▆▇▆▆▆▅▅▅▄▅▃▃▁▄▃▁▃▄▃▃▃▃▃▁▁▁▃▃▄▁▄▁▃▁▄▃▁▃▁▃▁▁▄▁▄▅▅▄▄▄▄ █
  5.34 ns      Histogram: log(frequency) by time      55.2 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

Presumably the dispatch is only triggered when it prints?

@wsshin
Copy link
Author

wsshin commented Apr 7, 2022

Just resolved the conflicts with the newly released Version 1.7.2.

Can this PR be merged now? This time I was able to resolve the conflicts with the newly released version, but I'm not sure if I could do so when the future versions introduce more complex conflicts. So, I hope this PR to be merged before the next version is released.

Please let me know if there are additional concerns. Note that

  • at least some people (me and @DilumAluthge) think the features implemented in this PR useful, and
  • the type instability issue raised by @martinholters turns out not causing any noticeable performance degradation, as demonstrated in this comment.

@christiangnrd
Copy link

What is the status on this pull request? I am very much looking forward to using this over percentage.

Copy link
Owner

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Sorry I was ignoring this and all my other packages while working on pkgimages for 1.9. This looks promising and I think we can merge it soon. I don't really expect a single runtime dispatch that happens only when printing occurs to be a big performance issue, but since the issue was raised perhaps we can get one more check?

@@ -66,6 +94,7 @@ mutable struct Progress <: AbstractProgress
tlast::Float64
printed::Bool # true if we have issued at least one status update
desc::String # prefix to the percentage, e.g. "Computing..."
rate_format::AbstractRateFormat
Copy link
Owner

Choose a reason for hiding this comment

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

More informative might be something like this:

julia> p = ProgressMeter.Progress(10000000; dt=0.001);

julia> @bprofile ProgressMeter.next!($p)
Progress: 100%|█████████████████████████████████████████████████████████████████████████████████| Time: 0:00:02
BenchmarkTools.Trial: 10000 samples with 1000 evaluations.
 Range (min  max):  5.342 ns  624.207 ns  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     6.455 ns               ┊ GC (median):    0.00%
 Time  (mean ± σ):   8.209 ns ±  15.537 ns  ┊ GC (mean ± σ):  0.00% ± 0.00%

  ▄█▄▄▂                                                       ▁
  ██████▆▅▆▇▆▆▆▅▅▅▄▅▃▃▁▄▃▁▃▄▃▃▃▃▃▁▁▁▃▃▄▁▄▁▃▁▄▃▁▃▁▃▁▁▄▁▄▅▅▄▄▄▄ █
  5.34 ns      Histogram: log(frequency) by time      55.2 ns <

 Memory estimate: 0 bytes, allocs estimate: 0.

Presumably the dispatch is only triggered when it prints?

@timholy
Copy link
Owner

timholy commented Aug 4, 2023

Current plan: replace this implementation with one that is type-stable. E.g.,

struct RateFormat
    format::Symbol
    digits::Int    # only used for certain `format`s
end

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.

5 participants