fix the versionchanged for frames #3710
Conversation
|
Hello @orbeckst! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-06-08 23:58:43 UTC |
Codecov Report
@@ Coverage Diff @@
## develop #3710 +/- ##
========================================
Coverage 94.35% 94.35%
========================================
Files 191 191
Lines 24984 24984
Branches 3375 3375
========================================
Hits 23574 23574
Misses 1362 1362
Partials 48 48
Continue to review full report at Codecov.
|
package/MDAnalysis/analysis/base.py
Outdated
| @@ -390,15 +390,18 @@ def run(self, start=None, stop=None, step=None, frames=None, | |||
| start frame of analysis | |||
| stop : int, optional | |||
| stop frame of analysis | |||
| step : int, optional | |||
| step : in, optional | |||
|
... fine, I'll fix the PEP8 issues. I am procrastinating anyway. |
- fix #3700 (fix incorrect versionchanged for new frames argument for base.AnalysisBase.run()) - updated docs to conform to Style Guide - additional logger.info output in run() when per-frame analysis starts - update CHANGELOG
orbeckst
force-pushed
the
fix-versionchanged-frames
branch
from
a87cb36 to
70b96e4
Compare
| @@ -23,6 +23,7 @@ Fixes | |||
| Enhancements | |||
| * Add `norm` parameter to InterRDF, InterRDF_s to normalize as rdf, | |||
| number density or do not normalize at all. (Issue #3687) | |||
| * Additional logger.info output when per-frame analysis starts (PR #3710) | |||
There was a problem hiding this comment.
It's minor but might actually be useful if one wants to see in the log when the per-frame loop starts and stops (that's one way to get timing information even if one only enabled logging).
| Support for setting ``start``, ``stop``, and ``step`` has been | ||
| removed. These should now be directly passed to | ||
| :meth:`AnalysisBase.run`. | ||
| Support for setting `start`, `stop`, and `step` has been removed. These |
There was a problem hiding this comment.
use 79 character line width
| @@ -452,7 +470,7 @@ class AnalysisFromFunction(AnalysisBase): | |||
| Raises | |||
| ------ | |||
| ValueError | |||
| if ``function`` has the same ``kwargs`` as :class:`AnalysisBase` | |||
| if `function` has the same `kwargs` as :class:`AnalysisBase` | |||
There was a problem hiding this comment.
follow Style Guide and reference args and kwargs with single backticks
| logger.info("Starting analysis loop over %d trajectory frames", | ||
| self.n_frames) |
There was a problem hiding this comment.
We announce frame selection, preparation and conclusion but not the main analysis loop. Adding this logger call makes it visible in a log file and allows reasonable ad-hoc benchmarking because the number of frames analyzed is also printed: one can calculate ((time of logger call conclusions) - (time of call starting analysis))/n_frames.
|
Apologies to @richardjgowers and @hmacdope , I made a few more changes (see summary and comments) and squashed everything. Please let me know if anything needs changing. If not, feel free to merge (assuming CI is happy). |
|
Nothing to change that I could see. Cheers @orbeckst |
Fixes #3700
Changes made in this Pull Request:
PR Checklist