alashworth / test-issue-import

0 stars 0 forks source link

time-based refreshes #117

Open alashworth opened 5 years ago

alashworth commented 5 years ago

Issue by bgoodri Thursday Sep 15, 2016 at 17:18 GMT Originally opened as https://github.com/stan-dev/stan/issues/2070


Summary:

Stan should output progress every X seconds rather than every X% of iterations

Description:

Stan should output progress every X seconds rather than every X% of iterations

https://groups.google.com/forum/#!searchin/stan-dev/%22time-based%22%7Csort:relevance/stan-dev/za8SWSVugTo/TCE8t6peFwAJ

Reproducible Steps:

Estimate a model

Current Output:

SAMPLING FOR MODEL 'lm' NOW (CHAIN 1).

Chain 1, Iteration:    1 / 2000 [  0%]  (Warmup)
Chain 1, Iteration:  200 / 2000 [ 10%]  (Warmup)
Chain 1, Iteration:  400 / 2000 [ 20%]  (Warmup)
Chain 1, Iteration:  600 / 2000 [ 30%]  (Warmup)
Chain 1, Iteration:  800 / 2000 [ 40%]  (Warmup)
Chain 1, Iteration: 1000 / 2000 [ 50%]  (Warmup)
Chain 1, Iteration: 1001 / 2000 [ 50%]  (Sampling)
Chain 1, Iteration: 1200 / 2000 [ 60%]  (Sampling)
Chain 1, Iteration: 1400 / 2000 [ 70%]  (Sampling)
Chain 1, Iteration: 1600 / 2000 [ 80%]  (Sampling)
Chain 1, Iteration: 1800 / 2000 [ 90%]  (Sampling)
Chain 1, Iteration: 2000 / 2000 [100%]  (Sampling)
 Elapsed Time: 0.795202 seconds (Warm-up)
               0.421444 seconds (Sampling)
               1.21665 seconds (Total)

Expected Output:

 Elapsed Time: 0.795202 seconds (Warm-up)
               0.421444 seconds (Sampling)
               1.21665 seconds (Total)

if X > 1.21665

Additional Information:

The current refresh behavior is sufficiently stupid and isolated from anything statistical that we should just get rid of it immediately without deprecation messages. I would be in favor of reinterpreting the integer passed to the refresh argument as a number of seconds. That way any existing code that specifies refresh would continue to work, although we probably need a higher default than 10 seconds. Alternatively, we could introduce a refresh_seconds argument and make people specify that explicitly to get the new behavior but then they won't get the new behavior unless they RTFM. Or we could introduce a refresh_seconds and just output a nonfatal message if the user specifies refresh saying that refresh is ignored.

Current Version:

v2.12.0

alashworth commented 5 years ago

Comment by ariddell Thursday Sep 15, 2016 at 20:24 GMT


This is a good point. Ultimately I think this kind of thing is going to be the interface's responsibility. On the other hand, if Stan wants to emit some estimate of iter/sec every 10 seconds, that would be useful information for any interface interested in keeping the user informed about what's going on.

alashworth commented 5 years ago

Comment by bgoodri Thursday Sep 15, 2016 at 20:32 GMT


Interfaces responsibility to do what? All we need to do is look the interface's stream up and it will print. The problem is right now we are printing essentially useless info at an inconvenient frequency. I don't think we should be giving anyone a choice to see useless "next 10% of iterations completed" once we have time-based refreshes.

On Thu, Sep 15, 2016 at 4:24 PM, Allen Riddell notifications@github.com wrote:

This is a good point. Ultimately I think this kind of thing is going to be the interface's responsibility. On the other hand, if Stan wants to emit some estimate of iter/sec every 10 seconds, that would be useful information for any interface interested in keeping the user informed about what's going on.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/stan/issues/2070#issuecomment-247443936, or mute the thread https://github.com/notifications/unsubscribe-auth/ADOrqic-V5HBo4TQ88L4jwIcAddQEeloks5qqamYgaJpZM4J-Hid .

alashworth commented 5 years ago

Comment by bob-carpenter Thursday Sep 15, 2016 at 21:56 GMT


What should time-based refreshes print in the way of progress and estimates of completion time?

Also, can we roll chain ID print out into this issue or another one? The parallel output is very confusing as is.

alashworth commented 5 years ago

Comment by ariddell Thursday Sep 15, 2016 at 22:10 GMT


Interfaces' responsibility to figure out how to display the information to the user. For example, in Jupyter Notebooks (and RStudio?) there's not really a standard out, so feedback needs to be provided in some other way. I could easily imagine a situation where the interface has a timer running in a separate thread which updates the display even 10 seconds even if Stan hasn't provided any new information (samples, messages, etc).

The services refactor also changes the way these messages are sent out -- there's a callback instead of an iostream. Might be worth revisiting after that lands.

On 09/15/2016 04:32 PM, bgoodri wrote:

Interfaces responsibility to do what? All we need to do is look the interface's stream up and it will print. The problem is right now we are printing essentially useless info at an inconvenient frequency. I don't think we should be giving anyone a choice to see useless "next 10% of iterations completed" once we have time-based refreshes.

On Thu, Sep 15, 2016 at 4:24 PM, Allen Riddell notifications@github.com wrote:

This is a good point. Ultimately I think this kind of thing is going to be the interface's responsibility. On the other hand, if Stan wants to emit some estimate of iter/sec every 10 seconds, that would be useful information for any interface interested in keeping the user informed about what's going on.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/stan/issues/2070#issuecomment-247443936, or mute the thread

https://github.com/notifications/unsubscribe-auth/ADOrqic-V5HBo4TQ88L4jwIcAddQEeloks5qqamYgaJpZM4J-Hid .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/stan-dev/stan/issues/2070#issuecomment-247446476, or mute the thread https://github.com/notifications/unsubscribe-auth/AABN7tKeehLCBptIL5zPBm67u5TTKiTYks5qqatmgaJpZM4J-Hid.

alashworth commented 5 years ago

Comment by sakrejda Thursday Sep 15, 2016 at 22:10 GMT


Could we just use newline separated json or yaml here if we're changing output? Otherwise we're begging for interfaces to implement some sort of awful character counting solution for pulling out timing info.

On Thu, Sep 15, 2016, 5:56 PM Bob Carpenter notifications@github.com wrote:

What should time-based refreshes print in the way of progress and estimates of completion time?

Also, can we roll chain ID print out into this issue or another one? The parallel output is very confusing as is.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/stan/issues/2070#issuecomment-247466770, or mute the thread https://github.com/notifications/unsubscribe-auth/AAfA6YQy43nzxnPD-gR5lBPaKkG7t9BQks5qqb72gaJpZM4J-Hid .

alashworth commented 5 years ago

Comment by bob-carpenter Thursday Sep 15, 2016 at 23:25 GMT


This would just change the output callbacks. And it'd require input arguments for how to control output frequency and/or number of output writes. But it wouldn't be some character counting things for interfaces.

As Allen says, it'll be up to the interfaces as to how to display output given the callbacks. You can't write to std::cout in R; instead, R provides a std::ostream pointer to which you can write. I would imagine Jupyter would then pick up whatever R writes.

On Sep 15, 2016, at 6:10 PM, Allen Riddell notifications@github.com wrote:

Interfaces' responsibility to figure out how to display the information to the user. For example, in Jupyter Notebooks (and RStudio?) there's not really a standard out, so feedback needs to be provided in some other way. I could easily imagine a situation where the interface has a timer running in a separate thread which updates the display even 10 seconds even if Stan hasn't provided any new information (samples, messages, etc).

The services refactor also changes the way these messages are sent out -- there's a callback instead of an iostream. Might be worth revisiting after that lands.

On 09/15/2016 04:32 PM, bgoodri wrote:

Interfaces responsibility to do what? All we need to do is look the interface's stream up and it will print. The problem is right now we are printing essentially useless info at an inconvenient frequency. I don't think we should be giving anyone a choice to see useless "next 10% of iterations completed" once we have time-based refreshes.

On Thu, Sep 15, 2016 at 4:24 PM, Allen Riddell notifications@github.com wrote:

This is a good point. Ultimately I think this kind of thing is going to be the interface's responsibility. On the other hand, if Stan wants to emit some estimate of iter/sec every 10 seconds, that would be useful information for any interface interested in keeping the user informed about what's going on.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/stan-dev/stan/issues/2070#issuecomment-247443936, or mute the thread

https://github.com/notifications/unsubscribe-auth/ADOrqic-V5HBo4TQ88L4jwIcAddQEeloks5qqamYgaJpZM4J-Hid .

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/stan-dev/stan/issues/2070#issuecomment-247446476, or mute the thread https://github.com/notifications/unsubscribe-auth/AABN7tKeehLCBptIL5zPBm67u5TTKiTYks5qqatmgaJpZM4J-Hid.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.