improved specifications for keyword arguments of run method #3191

amdnsr · 2021-03-28T21:05:59Z

Changes made in this Pull Request:

added kwargs in run method
made verbose a part of kwargs before passing it to ProgressBar constructor

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

codecov · 2021-03-28T21:24:24Z

Codecov Report

Merging #3191 (f5dbfbf) into develop (e32ce97) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head f5dbfbf differs from pull request most recent head 6834541. Consider uploading reports for the commit 6834541 to get more accurate results

@@           Coverage Diff            @@
##           develop    #3191   +/-   ##
========================================
  Coverage    92.73%   92.73%           
========================================
  Files          168      168           
  Lines        22683    22684    +1     
  Branches      3213     3213           
========================================
+ Hits         21035    21036    +1     
  Misses        1600     1600           
  Partials        48       48

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/base.py	`95.50% <100.00%> (+0.05%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e32ce97...6834541. Read the comment docs.

amdnsr · 2021-03-28T21:39:48Z

I preferred not to delete the argument verbose, because this method might have been already used with verbose as the positional argument (fourth argument) i.e. without the keyword verbose.

amdnsr · 2021-03-29T16:09:04Z

Please provide me some feedback on the pull request.
Thank you

IAlibay · 2021-03-29T16:18:47Z

@amdnsr one of the @MDAnalysis/gsoc-mentors will review your PR as soon as they can. Reviews often take a few days as all developers on the project are volunteers, so we often have our own work priorities that come before MDAnalysis.

lilyminium

Thanks for looking at this, @amdnsr. Please add a test so you can know that your changes work. You should also document kwargs in the docstring and update CHANGELOG (message of what your fix is, and your GitHub username on top). As this is your first contribution to MDAnalysis, please also add yourself to AUTHORS.

lilyminium · 2021-03-31T01:05:11Z

package/MDAnalysis/analysis/base.py

        for i, ts in enumerate(ProgressBar(
                self._trajectory[self.start:self.stop:self.step],
-                verbose=verbose)):
+                **kwargs)):


May I ask why you didn't simply verbose=verbose, **kwargs? That way you don't have to modify the kwargs dictionary.

Hey, I tried that, and yes, I won't have to modify the kwargs dictionary in the run function of base.py file, but the kwargs dictionary in the __init__ function of log.py file is expecting verbose to be a key in its kwargs dictionary, but in the suggested way, verbose will be a named parameter, and not a key in the kwargs dictionary of the __init__ function.
So, I would have to instead do the modifications over there.
This modification seemed cleaner to me, thus, I did it this way, but if the suggested way (verbose=verbose, **kwargs) is preferred, I will do it that way instead.

Also, please guide me on creating the tests. From what I understand, I need to create a function at the end of the testsuite/MDAnalysisTests/analysis/test_base.py file, that calls the ProgressBar, with some kwargs, which will in turn be passed to tqdm, and consequently, affect the position of each individual ProgressBar.
But, I can't figure out how and what set of methods/functions to call to do it, and how to verify whether my function is behaving as expected.
Thanks

Have a look at the tqdm documentation for possible parameters you could pass through, and the existing ProgressBar tests in MDAnalysis for suggestions on how you could test for progress bar appearance. We also have a brief guide for writing tests in the user guide.

Hey, I tried that, and yes, I won't have to modify the kwargs dictionary in the run function of base.py file, but the kwargs dictionary in the __init__ function of log.py file is expecting verbose to be a key in its kwargs dictionary, but in the suggested way, verbose will be a named parameter, and not a key in the kwargs dictionary of the __init__ function.
So, I would have to instead do the modifications over there.

I don't think I understand what you mean here. Could you show me your suggested changes? If you just pass in verbose=verbose, **kwargs, the log.ProgressBar class will consider both of them to be in kwargs due to dictionary unpacking

IAlibay · 2021-04-06T17:49:47Z

@amdnsr the deadline for GSoC applications is rapidly approaching. Are you still interested in completing this?

IAlibay · 2021-04-06T19:42:27Z

package/CHANGELOG

@@ -13,6 +13,22 @@ The rules for this file:
  * release numbers follow "Semantic Versioning" http://semver.org

 ------------------------------------------------------------------------------
+04/06/21 amdnsr


There's already a 2.0.0 entry below, you should only need to add your entry there.

Apologies, I did not get it. Should I add my changes just above the following line?
09/05/19 IAlibay, richardjgowers

0.20.1

No, there's already an existing 2.0.0 entry just below, this is where it should go.

…kwargs in the docstring.

IAlibay

@amdnsr just a reminder that there's only a few days left before the GSoC deadline. You'll need to address @lilyminium's comments in order for this PR to progress.

IAlibay · 2021-04-10T10:10:59Z

package/AUTHORS

@@ -155,7 +155,7 @@ Chronological list of authors
 2021
  - Aditya Kamath
  - Leonardo Barneschi
-
+  - Ahmad Nasir
 External code


Please make sure that you keep the empty line between the author list and the "External code" heading.

IAlibay · 2021-04-22T18:00:51Z

@amdnsr are you still looking to work on this?

IAlibay · 2021-05-05T10:28:41Z

Closing PR as stale.

improved specifications for keyword arguments of run method

f5dbfbf

amdnsr marked this pull request as ready for review March 28, 2021 21:35

IAlibay added the GSOC Starter label Mar 29, 2021

lilyminium requested changes Mar 31, 2021

View reviewed changes

IAlibay reviewed Apr 6, 2021

View reviewed changes

Added myself to AUTHORS, modified CHANGELOG and added description of …

6834541

…kwargs in the docstring.

amdnsr force-pushed the develop branch from 3ace839 to 6834541 Compare April 6, 2021 20:26

IAlibay requested changes Apr 10, 2021

View reviewed changes

IAlibay added the close? Evaluate if issue/PR is stale and can be closed. label Apr 22, 2021

IAlibay closed this May 5, 2021

marinegor mentioned this pull request Mar 21, 2023

Add progress bar keywords #4085

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improved specifications for keyword arguments of run method #3191

improved specifications for keyword arguments of run method #3191

amdnsr commented Mar 28, 2021 •

edited

Loading

codecov bot commented Mar 28, 2021 •

edited

Loading

amdnsr commented Mar 28, 2021

amdnsr commented Mar 29, 2021

IAlibay commented Mar 29, 2021

lilyminium left a comment

lilyminium Mar 31, 2021

amdnsr Apr 6, 2021

amdnsr Apr 6, 2021 •

edited

Loading

lilyminium Apr 6, 2021

lilyminium Apr 6, 2021

IAlibay commented Apr 6, 2021

IAlibay Apr 6, 2021

amdnsr Apr 6, 2021

IAlibay Apr 6, 2021

IAlibay left a comment

IAlibay Apr 10, 2021

IAlibay commented Apr 22, 2021

IAlibay commented May 5, 2021

improved specifications for keyword arguments of run method #3191

improved specifications for keyword arguments of run method #3191

Conversation

amdnsr commented Mar 28, 2021 • edited Loading

PR Checklist

codecov bot commented Mar 28, 2021 • edited Loading

Codecov Report

amdnsr commented Mar 28, 2021

amdnsr commented Mar 29, 2021

IAlibay commented Mar 29, 2021

lilyminium left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amdnsr Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay commented Apr 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IAlibay commented Apr 22, 2021

IAlibay commented May 5, 2021

amdnsr commented Mar 28, 2021 •

edited

Loading

codecov bot commented Mar 28, 2021 •

edited

Loading

amdnsr Apr 6, 2021 •

edited

Loading