Skip to content
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.

Sign up for GitHub

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

improved specifications for keyword arguments of run method #3191

Closed
wants to merge 2 commits into from
Closed

improved specifications for keyword arguments of run method #3191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f5dbfbf
improved specifications for keyword arguments of run method
amdnsr Mar 28, 2021
6834541
Added myself to AUTHORS, modified CHANGELOG and added description of …
amdnsr Apr 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package/AUTHORS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ Chronological list of authors
2021
- Aditya Kamath
- Leonardo Barneschi

- Ahmad Nasir
External code
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

-------------

Expand Down
5 changes: 4 additions & 1 deletion package/CHANGELOG
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ The rules for this file:
??/??/?? tylerjereddy, richardjgowers, IAlibay, hmacdope, orbeckst, cbouy,
lilyminium, daveminh, jbarnoud, yuxuanzhuang, VOD555, ianmkenney,
calcraven，xiki-tempula, mieczyslaw, manuel.nuno.melo, PicoCentauri,
hanatok, rmeli, aditya-kamath, tirkarthi
hanatok, rmeli, aditya-kamath, tirkarthi, amdnsr

* 2.0.0

Expand Down Expand Up @@ -133,6 +133,9 @@ Enhancements
'protein' selection (#2751 PR #2755)
* Added an RDKit converter that works for any input with all hydrogens
explicit in the topology (Issue #2468, PR #2775)
* Added kwargs in the run method of analysis/base.py which can be passed to tqdm,
via the ProgressBar class, a (subclass of tqdm), to provide access to the
functionality provided by tqdm (Issue #3190, PR #3191)

Changes
* TPRParser now loads TPR files with `tpr_resid_from_one=True` by default,
Expand Down
9 changes: 7 additions & 2 deletions package/MDAnalysis/analysis/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ def _conclude(self):
"""
pass # pylint: disable=unnecessary-pass

def run(self, start=None, stop=None, step=None, verbose=None):
def run(self, start=None, stop=None, step=None, verbose=None, **kwargs):
"""Perform the calculation

Parameters
Expand All @@ -174,18 +174,23 @@ def run(self, start=None, stop=None, step=None, verbose=None):
number of frames to skip between each analysed frame
verbose : bool, optional
Turn on verbosity
kwargs : keyword arguments, which will be used to access the
underlying functionality in tqdm class, via ProgressBar class,
specifically, to adjust the location of the bar on the screen
"""
logger.info("Choosing frames to analyze")
# if verbose unchanged, use class default
verbose = getattr(self, '_verbose',
False) if verbose is None else verbose
kwargs['verbose'] = verbose

self._setup_frames(self._trajectory, start, stop, step)
logger.info("Starting preparation")
self._prepare()

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@amdnsr amdnsr Apr 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self._frame_index = i
self._ts = ts
self.frames[i] = ts.frame
Expand Down