-
Notifications
You must be signed in to change notification settings - Fork 663
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
add progress bars #4072
add progress bars #4072
Conversation
@SophiaRuan normally we use the tqdm package for progress bars rather than "progress bars" You will need to test on your local machines |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #4072 +/- ##
========================================
Coverage 93.58% 93.58%
========================================
Files 192 192
Lines 25132 25133 +1
Branches 4056 4056
========================================
+ Hits 23519 23520 +1
Misses 1093 1093
Partials 520 520
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
@hmacdope sorry to butt in... we do indeed have our own |
@SophiaRuan the |
@RMeli is correct. @SophiaRuan would you be able to share a screenshot showing that the progress bars work? Otherwise looks good to me! |
Hi @hmacdope, I am wondering how to check if progress bars work. I only used the |
You should set up an example piece of code (copying one of the tests is fine) and then run it to see if progress bars appear. |
You may need to use the verbose=True flag to run I can't remember. |
Hi @hmacdope, I tried to run the |
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.
Could you add a CHANGELOG entry?
03/20/23 SophiaRuan * 2.5.0 Changes * Add progress bar to track the progress of _conclude() functions (_conclude_simple() and _conclude_fft()) in msd.py (PR MDAnalysis#4072)
@SophiaRuan you need to add them in this same PR |
package/CHANGELOG
Outdated
@@ -13,7 +13,15 @@ The rules for this file: | |||
* release numbers follow "Semantic Versioning" http://semver.org | |||
|
|||
------------------------------------------------------------------------------ | |||
|
|||
|
|||
03/20/23 SophiaRuan |
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.
This needs to go with the other 2.5.0 entries, just add it to the list.
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.
Hi @hmacdope, I just added the change to the 2.5.0 entries.
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.
Would you also be able to addd your name to AUTHORS
if you havn't already. other than that looks good!
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.
Just a couple of small review comments on changelog formatting from me please.
package/CHANGELOG
Outdated
* Add progress bars to track the progress of _conclude() functions | ||
(_conclude_simple() and _conclude_fft()) in msd.py (Issue #4070, PR #4072) |
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.
Entries should be newest first (please see line 7 in the instructions at the top of the file). This helps us easily scan the ordered history of changes in the repository.
package/CHANGELOG
Outdated
@@ -50,6 +50,9 @@ Changes | |||
analysis.nucleicacids' NucPairDist and WatsonCrickDist classes has been | |||
removed. Please use the `results.pair_distances` and `times` attributes | |||
instead (Issue #3744) | |||
* Add progress bars to track the progress of _conclude() functions | |||
(_conclude_simple() and _conclude_fft()) in msd.py (Issue #4070, PR #4072) | |||
|
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.
Sorry this might look overly pedantic but we try to keep the empty line formatting fixed in this file.
package/CHANGELOG
Outdated
??/??/?? IAlibay, pgbarletta, mglagolev, hmacdope, manuel.nuno.melo, chrispfae, | ||
ooprathamm, MeetB7, BFedder, v-parmar, MoSchaeffler, jbarnoud, jandom, | ||
xhgchen, jaclark5, DrDomenicoMarson | ||
xhgchen, jaclark5, DrDomenicoMarson, SophiaRuan | ||
|
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.
I know this is annoying @SophiaRuan but this should show no diff except your name
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.
Hi @hmacdope, I just made some changes. Could you help me check a bit? Thank you for your help!
Hi @hmacdope, I made some changes to the Author and CHANGELOG recently. Could you help me check on this? |
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.
Apply this suggestion and should be good to go.
Co-authored-by: Hugo MacDermott-Opeskin <[email protected]>
Linter Bot Results:Hi @SophiaRuan! Thanks for making this PR. We linted your code and found the following: Some issues were found with the formatting of your code.
Please have a look at the Please note: The |
Hi @hmacdope, I have made the change! Thank you! |
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.
Looks good to me !
@SophiaRuan resolve conflicts and then good to go. |
Hi @hmacdope @IAlibay, I have solved the conflicts. Thank you for your help! |
Fixes #4070
Changes made in this Pull Request:
PR Checklist