Setting Verbose #2631
Setting Verbose #2631Purva-Chaudhari wants to merge 14 commits intoMDAnalysis:developfrom Purva-Chaudhari:develop
Conversation
Add Chemfiles as a coordinate reader/writer (#1862)
|
@lilyminium over-ridding needs to be done only when verbose is set to True in run (as i observed in some of the test cases) Hence i set it this way. But still if this doesn,t work i have another approach in mind . Do let me know, |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Some initial comments, probably will need some clarifications of the logic here, apologies if I'm just failing to understand what you are trying to do here.
package/MDAnalysis/analysis/base.py
Outdated
| self.start = start | ||
| self.stop = stop | ||
| self.step = step | ||
| if start is not None: |
There was a problem hiding this comment.
Quick question here, is there a case where check_slice_indices returns None for either of start, stop or step? As far as I was aware it already returned ints?
There was a problem hiding this comment.
I am sorry got confused with issue #2206 requirements asking to Adjust AnalysisBase.init() so that start/stop/step are stored in the respective private variables. Also make sure that the DeprecationWarning is raised even if any of the deprecated kwargs is None (currently missing).
Will remove that part for now and work on it after solving this one
package/MDAnalysis/analysis/base.py
Outdated
| elif step is not None: | ||
| self._step = step | ||
| else: | ||
| warnings.warn("Deprecation Warning", DeprecationWarning) |
There was a problem hiding this comment.
Aside from the above comment, I'm not sure I understand how this relates to setting verbose. Apologies if I'm just missing something simple, any chance you could provide extra information on why you're doing this?
package/MDAnalysis/analysis/base.py
Outdated
| # if verbose unchanged, use class default | ||
| verbose = getattr(self, '_verbose', False) if verbose is None else verbose | ||
|
|
||
| if verbose == True : self.verbose = verbose |
There was a problem hiding this comment.
Based on line 142, shouldn't this be self._verbose?
Also, I'm probably getting the logic wrong here, but wouldn't it be best to always set self._verbose here? What if you have a case where you pass False to run() but True to __init__, in this construct self._verbose would be set to True even if verbose is currently False right?
There was a problem hiding this comment.
In case of parameter overriding for the run class can it be directly:
verbose = getattr(self, '_verbose', False) if verbose is None else verbose
-> self._verbose = verbose after the line
There was a problem hiding this comment.
Please do correct me if i am going wrong in understanding the requirement. (Just re-read the comments regarding deprecation.)
Codecov Report
@@ Coverage Diff @@
## develop #2631 +/- ##
===========================================
- Coverage 91.00% 90.88% -0.12%
===========================================
Files 174 174
Lines 23550 23256 -294
Branches 3083 3077 -6
===========================================
- Hits 21431 21137 -294
Misses 1497 1497
Partials 622 622
Continue to review full report at Codecov.
|
@Purva-Chaudhari as far as I can tell this should work, you'll probably want to introduce a test though. Probably something similar to: mdanalysis/testsuite/MDAnalysisTests/analysis/test_base.py Lines 93 to 95 in 7cb4184 But testing the influence of verbose being set when calling |
|
I do wonder if it might be cheaper / safer to just explicitly pass Maybe one of the @MDAnalysis/coredevs might have some thoughts on this? |
|
I like @IAlibay 's idea to pass instead of setting/unsetting AnalysisBase._verbose. This looks like a clean solution that makes sure that run(..., verbose=VALUE) is only temporary.
|
|
@IAlibay so should I pass verbose explicitly to _setup_frames . In that case of temporary overriding, the line : verbose = getattr(self, '_verbose', False) in _setup_frames can be removed and i guess then we don,t have to set self._verbose in run/_setup_frames. Should I make this change and then introduce test? |
@Purva-Chaudhari Looks like the idea is popular, please do go ahead 👍 |
|
@IAlibay Could you please suggest me further |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
@Purva-Chaudhari Please see below for some documentation changes.
We also probably need some tests here. I can't think of a simple way to check that ProgressMeter was passed the right value (@orbeckst any thoughts here?). But at the very least we could check that calling run with a value that overrides the value set on object creation is temporary. As previously mentioned, I'd suggest this be an additional test in mdanalysis/testsuite/MDAnalysisTests/analysis/test_base.py, doing something similar to test_verbose, but checking _verbose after calling run(verbose=somevalue).
| def _setup_frames(self, trajectory, start=None, stop=None, step=None, verbose = None): | ||
| """ | ||
| Pass a Reader object and define the desired iteration pattern | ||
| through the trajectory |
There was a problem hiding this comment.
The docstring needs updating to reflect that verbose is now an explicit argument to _setup_frames. Please also include a .. versionchanged entry detailing that the verbose argument was added in 1.0.0.
package/MDAnalysis/analysis/base.py
Outdated
| interval = 1 | ||
|
|
||
| verbose = getattr(self, '_verbose', False) | ||
|
|
There was a problem hiding this comment.
Probably don't need two empty lines here.
| @@ -134,7 +134,7 @@ def _setup_frames(self, trajectory, start=None, stop=None, step=None): | |||
| if interval == 0: | |||
There was a problem hiding this comment.
Do also add a versionchanged to run to document this new behaviour (i.e. temporary overriding of __init__ set verbose).
|
@Purva-Chaudhari also don't forget to update CHANGELOG and AUTHORS! |
|
The verbosity is not a result, it should not be returned. Checking stdout or stderr would be better. |
|
@joaomcteixeira could you please also keep an eye on this PR and help out @IAlibay who is already doing many others. Thanks! |
|
Sorry for not seeing this earlier @Purva-Chaudhari. I think you should be able to check the verbosity of the ProgressMeter at |
@lilyminium The temporary overriding of verbose is done by passing it to |
|
@Purva-Chaudhari yes, you would be checking that the |
|
@Purva-Chaudhari can you solve your merge conflicts, that way we can see if Travis runs green. |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Starting to look good, some docstring changes, and an extension of the test.
| @@ -123,6 +123,9 @@ def _setup_frames(self, trajectory, start=None, stop=None, step=None): | |||
| stop frame of analysis | |||
| step : int, optional | |||
| number of frames to skip between each analysed frame | |||
There was a problem hiding this comment.
Please do add verbose to the parameter list please.
package/MDAnalysis/analysis/base.py
Outdated
| number of frames to skip between each analysed frame | ||
|
|
||
| .. versionchanged:: 1.0.0 | ||
| added verbose argument |
There was a problem hiding this comment.
Identation issue, should be:
.. versionchanged:: 1.0.0
Verbose argument is now passed explicitly from :meth:`AnalysisBase.run`.
package/MDAnalysis/analysis/base.py
Outdated
| Turn on verbosity | ||
|
|
||
| .. versionchanged:: 1.0.0 | ||
| verbose explicitly passed to _setup_frames as argument for temporary |
There was a problem hiding this comment.
Same as above, please do use :meth: and :class: docstring decorators too if you can, that way users can navigate to the right place when looking at the sphinx documentation.
|
|
||
|
|
||
| def test_verbose_run(u): | ||
| a = FrameAnalysis(u.trajectory, verbose=False).run(verbose = True) |
There was a problem hiding this comment.
PEP8 correction: no spaces around = please.
| assert a._verbose | ||
|
|
||
|
|
||
| def test_verbose_run(u): |
There was a problem hiding this comment.
Here it would be good to test multiple cases using pytest.mark.parametrize, i.e. the cases of:
init, run
True, True
False, True
True, False
False, False
Let us know if you need any help to work out how to use parametrize. See test_AnalysisFromFunction for an example of it.
There was a problem hiding this comment.
@pytest.mark.parametrize('verbose, verbose_r',[(True,True), (True,False), (False,True), (False,False)])
def test_verbose_run(u,verbose, verbose_r)
a = FrameAnalysis(u.trajectory, verbose=verbose).run(verbose = verbose_r)
(should I include the four conditions )
if (a._verbose == True and a._pm.verbose == True):
assert a._verbose
assert a._pm.verbose
elif (a._verbose == True and a._pm.verbose == False):
assert a._verbose
assert not a._pm.verbose
elif (a._verbose == False and a._pm.verbose == True):
assert not a._verbose
assert a._pm.verbose
elif (a._verbose == False and a._pm.verbose == False):
assert not a._verbose
assert not a._pm.verbose
or simply
assert a._verbose
assert a._pm.verbose
Please see if this is correct or no
There was a problem hiding this comment.
You don't need the if/else construct, just assert that the a._verbose and a._pm.verbose values match the expected inputs verbose and verbose_r.
Obviously, this is all dependent on #2631 (comment)
|
There is a pull request about replacing the progress meter to use tqmd. It is worth checking that the testing strategy remains valid. |
|
Unfortunately, I think #2617 actually makes this PR redundant since it never sets Edit: hopefully a better way to say this: PR #2617 removes the original problem in #2504 because the progress bar does not depend on |
|
That other PR should probably not change the API. We should check that. On 20 Mar 2020 11:18, Lily Wang <notifications@github.com> wrote:
Unfortunately, I think #2617 actually makes this PR redundant since it never sets verbose or passes it to _setup_frames.
—You are receiving this because you are on a team that was mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
What should I do further then? |
package/MDAnalysis/analysis/base.py
Outdated
| self._verbose = verbose | ||
|
|
||
| def _setup_frames(self, trajectory, start=None, stop=None, step=None): | ||
| def _setup_frames(self, trajectory, start=None, stop=None, step=None, verbose = None): |
There was a problem hiding this comment.
use verbose=None without spaces
|
Hi @Purva-Chaudhari , Just a small comment on to properly develop on a fork. I see you have made the changes in your forked |
joaomcteixeira
left a comment
joaomcteixeira
left a comment
There was a problem hiding this comment.
I have had a look at the implementation details. I could not evaluate the whole functionality at this time. Can anyone provide me a quick way to run an example on this? Thanks!
| :meth:`AnalysisBase.run`. | ||
| """ | ||
| self._trajectory = trajectory | ||
| start, stop, step = trajectory.check_slice_indices(start, stop, step) |
There was a problem hiding this comment.
I would not use temporary variables here. You create a place for a bug to happen.
directly assign the returned values from trajectory.check_slice_indeces to the self.start .stop and .step.
There was a problem hiding this comment.
and on line 139 use the self. attributes instead of the temporary variables. It looks innocent I know but with time these kinds of practices start to accumulate and having their tool.
There was a problem hiding this comment.
Since this is historical AnalysisBase stuff, @richardjgowers might be best placed to discuss if there were any specific reasons for this (I think it was added in #460).
This behaviour is probably going to need to change in light of #2206, so I'm not sure if it's not best to just tackle all this in a separate PR?
There was a problem hiding this comment.
These are valid comments, though these changes are out of scope of this pull request.
| interval = 1 | ||
|
|
||
| verbose = getattr(self, '_verbose', False) | ||
| self._pm = ProgressMeter(self.n_frames if self.n_frames else 1, |
There was a problem hiding this comment.
Python tip:
a = None or 1
# or
b = 0 or 77😉
| """ | ||
| logger.info("Choosing frames to analyze") | ||
| # if verbose unchanged, use class default | ||
| verbose = getattr(self, '_verbose', False) if verbose is None else verbose |
There was a problem hiding this comment.
To my understanding, there is no need to use getattr here. getattr and setattr are used on dynamic operations, not on static ones.
There was a problem hiding this comment.
Hi @Purva-Chaudhari ,
Just a small comment on to properly develop on a fork. I see you have made the changes in your forked
developbranch. This is not completely wrong, but it is best to branch out first to a new branch that addresses only the PR/issue in question. And then, Pull Request that branch to the main repository base. If you continue developing new features in the fork base branch, as per habit, it may get really problematic if you have to deal with complex merge conflicts in future situations.
Sorry was sort of new to open source, will ensure this henceforth
There was a problem hiding this comment.
No problem. I understood that. You did a very nice first attempt. 😉 that's why I commented on it.
There was a problem hiding this comment.
Here self._verbose may not be defined, so the getattr is necessary. Another possibility would be to wrap the call in a try, but it would be more cluttered.
There was a problem hiding this comment.
To my understanding, there is no need to use
getattrhere.getattrandsetattrare used on dynamic operations, not on static ones.
should I make it verbose = False if none else verbose ?
|
|
||
| def test_verbose_run(u): | ||
| a = FrameAnalysis(u.trajectory, verbose=False).run(verbose = True) | ||
| assert not a._verbose |
There was a problem hiding this comment.
assert a._verbose is False
There was a problem hiding this comment.
not a._verbose is correct. There is no reason to compare the identity with the False singleton; in theory, everything that is considered false by python would be a valid value here.
There was a problem hiding this comment.
Correct. Thanks for the feedback!
| verbose = getattr(self, '_verbose', False) if verbose is None else verbose | ||
|
|
||
| self._setup_frames(self._trajectory, start, stop, step) | ||
| self._setup_frames(self._trajectory, start, stop, step, verbose) |
There was a problem hiding this comment.
I don't think this is correct. Why should a method of a class receive an argument that is an attribute of that same class when the method being called is not a @staticmethod ? (may be this is not from this PR and I am missing something)
There was a problem hiding this comment.
start, stop, step, and verbose are historically argument of the class, though we are deprecating them from the initializer in favour of the same arguments in the run method.
There was a problem hiding this comment.
Sorry, I was referring to the self._trajectory. Because it also gets assigned again on _setup_frames. But this is for other PR/issue, indeed.
| def test_verbose_run(u): | ||
| a = FrameAnalysis(u.trajectory, verbose=False).run(verbose = True) | ||
| assert not a._verbose | ||
| assert a._pm.verbose |
|
@Purva-Chaudhari , PR #2617 (which will be merged soon) makes issue #2504 go away by design so I am sorry to say that this PR that you have been working on will be closed without merging. I understand that you and various mentors have put a lot of effort into it. This would have been your first contribution and in this way you would have qualified for submitting a GSoC proposal. From what I can see from this PR, we would have eventually merged it as it was close to acceptance. So I am going to say that we will accept your GSoC proposal submission on the basis of your work seen in this PR #2631 unless you get another PR merged by 3/31/2020 (and we obviously encourage you to do so). — @orbeckst for @MDAnalysis/gsoc-mentors |
|
@orbeckst Thank you for your acceptance regarding proposal, wanted to ask if I could work on an issue not labelled as gsoc starter. I see many of the gsoc labelled issues already taken. Any suggestions regarding any small issue to get solved please let me know, |
6 participants
Fixes #2504
Changes made in this Pull Request:
PR Checklist