-
-
Notifications
You must be signed in to change notification settings - Fork 409
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
Adding Progress Bars #1703
Adding Progress Bars #1703
Conversation
151d490
to
e788257
Compare
Before a pull request is accepted, it must meet the following criteria:
|
Codecov Report
@@ Coverage Diff @@
## master #1703 +/- ##
==========================================
- Coverage 62.65% 62.63% -0.02%
==========================================
Files 65 65
Lines 6036 6089 +53
==========================================
+ Hits 3782 3814 +32
- Misses 2254 2275 +21
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The progress bars look great, but I personally think this should not be a separate PR. To me, this is still part of the convergence plots because it's showing the progress of the model. I would like to see the layout of the progress bars and convergence plots together.
What is the impact on performance using these bars, if any?
Minor changes:
- The packets bar should read "XX% of packets propagated". Perhaps even "XX% of real packets propagated".
- There are hardcoded values for iterations and packet numbers, but these should come from the input yaml file. You can get the total packet number from (iterations - 1) * no_of_packets + last_no_of_packets
Thank you @MarkMageeAstro! |
That's great, I would consider a ~10% increase acceptable. |
from tqdm import tqdm | ||
|
||
packet_pbar = tq.tqdm( | ||
total=860000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't hardcode the number of packets or iterations. These numbers need to be retrieved from the tardis config object.
bar_format="{bar}{percentage:3.0f}% packets propagated", | ||
) | ||
iteration_pbar = tq.tqdm( | ||
total=20, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Retrieve number of iterations from the tardis config object. This is a user specified parameter.
with objmode: | ||
update_packet_pbar(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try using if (i % 1000 == 0)
as a condition. You might need to also change the argument in update_packet_pbar
to 1000. Use the tqdm documentation here and click the links to the source code for the update function:
iterations = 20 | ||
packets = 860000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These values need to be read in from the config dict.
9fe7e2f
to
4704edc
Compare
7d2d6ae
to
4699251
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The progress bar looks nice and code quality looks ok to me (considering you haven't added docstrings to existing functions that didn't have them already). Rest about code changes - since it deals with numba I'd request @andrewfullard to have a look.
Have you checked how progress bar looks on static html page (in docs)?
183e49a
to
f7b1135
Compare
this has preliminary approval from me (pending the others approval) - it looks very nice. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @atharva-2001. Everything looks good except mentioning about it in our docs is missing.
AFAIR, you were adding a "Progress bar for Simulation Run" page in "Additional outputs" section explaining options related to progress bar, different modes of display, etc. along with a note in quickstart (?) that progress bar doesn't get displayed in static page but will show up in interactive mode?
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
1cebfdc
to
9bd9125
Compare
e842526
to
54d82ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great addition to the code!
* Customize bar format * Display bars again after reset when full * Get data from configuration file * Track all packets at once and don't reset the progress bar * Close last container and add docstrings * Add option to enable/disble the progress bar * Display container only in notebooks * Add a progress bar to track iterations * Align bars * Refactor code * Format using black * Add message regarding progress bars in the quickstart notebook * Demonstrate progress bars in the documentation * Change show_progress_bar to show_progress_bars * [build docs]
* Customize bar format * Display bars again after reset when full * Get data from configuration file * Track all packets at once and don't reset the progress bar * Close last container and add docstrings * Add option to enable/disble the progress bar * Display container only in notebooks * Add a progress bar to track iterations * Align bars * Refactor code * Format using black * Add message regarding progress bars in the quickstart notebook * Demonstrate progress bars in the documentation * Change show_progress_bar to show_progress_bars * [build docs]
* Customize bar format * Display bars again after reset when full * Get data from configuration file * Track all packets at once and don't reset the progress bar * Close last container and add docstrings * Add option to enable/disble the progress bar * Display container only in notebooks * Add a progress bar to track iterations * Align bars * Refactor code * Format using black * Add message regarding progress bars in the quickstart notebook * Demonstrate progress bars in the documentation * Change show_progress_bar to show_progress_bars * [build docs]
This PR adds progress bars to TARDIS.
Description
Motivation and context
How has this been tested?
Examples
Please visit the documentation here:
https://atharva-2001.github.io/tardis/branch/progress_bar/io/output/index.html
https://atharva-2001.github.io/tardis/branch/progress_bar/quickstart/quickstart.html
Type of change
Checklist