Fixing large stdout issue #134

iomaganaris · 2019-02-19T13:48:45Z

Added an interval between progress bar prints
Max number of progress bar prints set to 1000
Keeps ETA updated regularly
Fixes Avoid large stdout from long running simulations due to progressbar #116

coreneuron/utils/progressbar/progressbar.c

ferdonline

Hey. Here goes another review. I think I missed the logic, tomorrow we can look into it, but a general comment would be for more informative var names.
Also feel free to put these associated constants in the same enum

coreneuron/utils/progressbar/progressbar.c

ferdonline · 2019-02-19T21:42:40Z

coreneuron/utils/progressbar/progressbar.c

+enum { BAR_UPDATES_INTERVAL = 5 };
+
+/// Progress Bar time of change of updates (seconds)
+enum { BAR_UPDATES_CHANGE = 120 };


more suggestive names:

/// The reference bar redraw interval BAR_DRAW_INTERVAL = 5 // maybe (isatty()? 1 : 5) /// The maximum number of updates (avoid frequent output on very long runs ) BAR_DRAW_COUNT_MAX = 500 /// The Initial period, where BAR_DRAW_INTERVAL is always respected BAR_DRAW_FREQUENT_PERIOD = 120

Thank you very much. I will take care of this and try to be more descriptive the next time 👍

@ferdonline Please check the next commit and give me some feedback if you want on the variables' names descriptiveness.

ferdonline

Hi @iomaganaris
Thanks for the work you did here.
The idea of sampling the ETA is indeed nice! (I even wonder if we are getting too excited about it)
Besides the code comment, my thoughts are:

1000 samples seems a lot for me. Maybe ask one of our mathematicians, but I have the impression that to get a good average from a distribution we need actually very few samples (kind, less than 50...).
If the simulation gets heavier and heavier with time, even a lot of samples won't represent the mean. (Maybe Pramod knows if that's a common scenario or not). In case it is, a moving average would be better (of course, this is if we really want to be fancy ;) )
If we can stay on the simple side, maybe we could have sample_interval = draw_time_interval and simplify those if's.

But anyway this is just reflexion. I'm more than ok with current implementation.
I didn't test this on a real execution though. If you tested that feel free to merge.

coreneuron/utils/progressbar/progressbar.c

- Added an interval between progress bar prints - Max number of progress bar prints set to 1000 - Keeps ETA updated regularly - Fixes #116

- Progress bar will be printed every BAR_UPDATES_INTERVAL seconds for the first BAR_UPDATES_CHANGE seconds and then output max BAR_UPDATES_NUMBER progress bars. - Need to check performance difference due to if-statements and different number of progress bar prints

Progress bar will be drawn every BAR_DRAW_INTERVAL seconds for the first BAR_DRAW_INIT_PERIOD seconds (Initial period) and then choose between drawing every BAR_DRAW_INTERVAL seconds or draw the bar BAR_DRAW_REMAINING_UPDATES times, depending on which option draws the progress bar less times. - Need to save the last calculated eta and bar->value of the Initial period to calculate correctly the BAR_DRAW_COUNT_REMAINING and to redraw progress bar max BAR_DRAW_COUNT_MAX times in total. - Changed the variable names to be more descriptive - Fixed BAR_DRAW_COUNT_REMAINING computation - Fixed printing '\r' in each progress bar drawing

Sample ETA value throughout the simulation to calculate the time intervals between progress bar redraws based on the max number of progress bar drawings wanted. ETA sampling makes the time intervals for redrawing adapt to the time the simulation takes while avoiding ETA extreme values in the first simulation steps or the end of the simulation. Up until the first ETA sample, progress bar is redrawn every 2 seconds. The total number of progress bars drawn may surpass the BAR_DRAW_COUNT_MAX, but only by a little due to the ETA forecasting variability. - Needs discussion about the number of samples of ETA and if the small variance in the total number of progress bars drawn is important - Needs checking with some more corner use cases

- Sample ETA every time the progress bar is redrawn - Flush stdout every time the progress bar is redraw to be sure that the bar is printed in output every time - Fixes #116

- Moved eta_s variable calculation to be done only if needed - Return from progress_bar update func if the progress bar must not be redrawn

…lueBrain/CoreNeuron#134) CoreNEURON Repo SHA: BlueBrain/CoreNeuron@1fcd9c2

iomaganaris requested review from pramodk and ferdonline February 19, 2019 13:48

ferdonline suggested changes Feb 19, 2019

View reviewed changes

coreneuron/utils/progressbar/progressbar.c Outdated Show resolved Hide resolved

coreneuron/utils/progressbar/progressbar.c Outdated Show resolved Hide resolved

iomaganaris requested a review from jorblancoa February 19, 2019 17:24

ferdonline reviewed Feb 19, 2019

View reviewed changes

iomaganaris force-pushed the large_stdout branch 2 times, most recently from dfa52df to 30eb013 Compare February 20, 2019 17:00

ferdonline approved these changes Feb 21, 2019

View reviewed changes

coreneuron/utils/progressbar/progressbar.c Outdated Show resolved Hide resolved

ferdonline approved these changes Feb 25, 2019

View reviewed changes

pramodk approved these changes Feb 26, 2019

View reviewed changes

iomaganaris and others added 10 commits February 26, 2019 21:09

Fixing large stdout issue

24472a9

- Added an interval between progress bar prints - Max number of progress bar prints set to 1000 - Keeps ETA updated regularly - Fixes #116

Fixed if-statements variable names

4b95bd2

Small explanation comment correction

a1cc202

Improving the ETA sampling mechanism

9596f0b

- Sample ETA every time the progress bar is redrawn - Flush stdout every time the progress bar is redraw to be sure that the bar is printed in output every time - Fixes #116

Minor design fixes

9d4cadc

- Moved eta_s variable calculation to be done only if needed - Return from progress_bar update func if the progress bar must not be redrawn

Slower default rate when not TTY. Little polishing

fb68efc

Initial conditions

c3012fa

pramodk force-pushed the large_stdout branch from 1d77ffc to c3012fa Compare February 26, 2019 20:12

pramodk merged commit 1fcd9c2 into master Feb 26, 2019

pramodk deleted the large_stdout branch February 26, 2019 20:18

iomaganaris added a commit that referenced this pull request Mar 6, 2019

Avoid large stdout with progress bar fixes #116 (#134)

7bc804e

orena1 mentioned this pull request Feb 11, 2020

Fix issue with progress bar not showing in gap junction simulations #254

Merged

pramodk pushed a commit to neuronsimulator/nrn that referenced this pull request Nov 2, 2022

Avoid large stdout with progress bar fixes BlueBrain/CoreNeuron#116 (B…

c420789

…lueBrain/CoreNeuron#134) CoreNEURON Repo SHA: BlueBrain/CoreNeuron@1fcd9c2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixing large stdout issue #134

Fixing large stdout issue #134

Uh oh!

iomaganaris commented Feb 19, 2019

Uh oh!

Uh oh!

Uh oh!

ferdonline left a comment

Uh oh!

Uh oh!

Uh oh!

ferdonline Feb 19, 2019 •

edited

Loading

Uh oh!

iomaganaris Feb 19, 2019

Uh oh!

iomaganaris Feb 20, 2019

Uh oh!

ferdonline left a comment

Uh oh!

Uh oh!

Uh oh!

Fixing large stdout issue #134

Fixing large stdout issue #134

Uh oh!

Conversation

iomaganaris commented Feb 19, 2019

Uh oh!

Uh oh!

Uh oh!

ferdonline left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ferdonline Feb 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iomaganaris Feb 19, 2019

Choose a reason for hiding this comment

Uh oh!

iomaganaris Feb 20, 2019

Choose a reason for hiding this comment

Uh oh!

ferdonline left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ferdonline Feb 19, 2019 •

edited

Loading