removed quiet in favour of verbose by richardjgowers · Pull Request #2055 · MDAnalysis/mdanalysis

richardjgowers · 2018-08-22T19:04:59Z

Fixes #1975 #1979
Starts #1463

Changes made in this Pull Request:

PR Checklist

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

orbeckst · 2018-08-31T05:48:30Z

That fails with some _set_verbose still being used somewhere.

orbeckst · 2018-09-11T03:58:50Z

You need to merge or rebase against develop because #2066 was "fixed".

orbeckst · 2018-09-12T01:02:46Z

@richardjgowers I rebase this branch against develop and force-pushed. Be careful with your next git pull.

richardjgowers · 2018-09-13T22:45:52Z

Ok I still need to

check if/where we refer to start/stop/step in docs
check existing usage of start/stop/step in tests etc
add tests for the new deprecations
check I got verbose right

richardjgowers · 2018-09-14T16:29:47Z

package/MDAnalysis/analysis/base.py

+                deps.append(arg)
+                setattr(self, arg, kwargs[arg])
+        if deps:
+            raise ValueError(deps)


I've made this a VE so that I can find all deprecated usages in the testsuite, will remove later..

package/MDAnalysis/analysis/pca.py

richardjgowers · 2018-09-14T16:32:13Z

package/MDAnalysis/analysis/pca.py

        """
        if not self._calculated:
-            self.run()
+            self.run(start=start, stop=stop, step=step)


this seems to cause errors. Are run and transform meant to be allowed to use different frames? @kain88-de

Yes. You may first compute the transformation and then apply it to an arbitrary trajectory to project out the motions corresponding to the PCs.

I can't think of a clean way to separate them here so instead of running self.run() rather raise the error and let the user do it explicitly. Also makes for much clearer scripts on the user side.

Needs to be documented in the CHANGELOG and with a

.. versionchanged 0.19.0:: :meth:`transform` does not automatically :meth:`run` the PCA if needed; instead it raises :exc:`ValueError` to remind the user to perform :meth:`run` first.

orbeckst

comments on PCA; @kain88-de and @jdetle – feel free to correct me.

package/MDAnalysis/analysis/pca.py

orbeckst · 2018-09-14T18:18:35Z

package/MDAnalysis/analysis/pca.py

        """
        if not self._calculated:
-            self.run()
+            self.run(start=start, stop=stop, step=step)


Yes. You may first compute the transformation and then apply it to an arbitrary trajectory to project out the motions corresponding to the PCs.

I can't think of a clean way to separate them here so instead of running self.run() rather raise the error and let the user do it explicitly. Also makes for much clearer scripts on the user side.

Needs to be documented in the CHANGELOG and with a

.. versionchanged 0.19.0:: :meth:`transform` does not automatically :meth:`run` the PCA if needed; instead it raises :exc:`ValueError` to remind the user to perform :meth:`run` first.

jdetle · 2018-09-14T18:34:52Z

codecov · 2018-09-18T21:54:46Z

Codecov Report

Merging #2055 into develop will increase coverage by 0.01%.
The diff coverage is 82.81%.

@@             Coverage Diff             @@
##           develop    #2055      +/-   ##
===========================================
+ Coverage    89.33%   89.34%   +0.01%     
===========================================
  Files          159      159              
  Lines        18729    18712      -17     
  Branches      2684     2684              
===========================================
- Hits         16731    16719      -12     
+ Misses        1396     1387       -9     
- Partials       602      606       +4

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/contacts.py	`97.56% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/density.py	`64.94% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/helanal.py	`85.24% <100%> (-0.05%)`	⬇️
package/MDAnalysis/analysis/rms.py	`90.66% <100%> (-0.36%)`	⬇️
package/MDAnalysis/analysis/waterdynamics.py	`81.13% <100%> (+0.19%)`	⬆️
...ckage/MDAnalysis/analysis/hbonds/hbond_analysis.py	`80.64% <100%> (+0.18%)`	⬆️
package/MDAnalysis/analysis/diffusionmap.py	`90.62% <100%> (ø)`	⬆️
package/MDAnalysis/lib/log.py	`97.7% <100%> (-0.22%)`	⬇️
package/MDAnalysis/core/universe.py	`95.04% <100%> (-0.02%)`	⬇️
...age/MDAnalysis/analysis/hbonds/wbridge_analysis.py	`86.45% <71.42%> (+0.46%)`	⬆️
... and 6 more

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 259f778...21843d3. Read the comment docs.

richardjgowers · 2018-09-18T21:55:07Z

@orbeckst this is hopefully finished

orbeckst

Looking good, thanks for all the work. Just update the CHANGELOG, please.

orbeckst · 2018-09-18T22:00:02Z

package/CHANGELOG

+  * PCA.transform now requires that PCA.run has already been called (PR #2055)

 Deprecations
  * Almost all "save()", "save_results()", "save_table()" methods in


add start/stop/step deprecations here

orbeckst · 2018-09-18T22:00:20Z

package/CHANGELOG

    enforced to be of the form ``[lx, ly, lz, alpha, beta, gamma]`` as required
    by the docs (Issue #2046, PR #2048)
  * Added periodic keyword to select_atoms (#759)
+  * PCA.transform now requires that PCA.run has already been called (PR #2055)


add start/stop/step in run() here

orbeckst · 2018-09-18T22:01:03Z

package/CHANGELOG

    enforced to be of the form ``[lx, ly, lz, alpha, beta, gamma]`` as required
    by the docs (Issue #2046, PR #2048)
  * Added periodic keyword to select_atoms (#759)
+  * PCA.transform now requires that PCA.run has already been called (PR #2055)


Add removal of quiet here

orbeckst · 2018-09-18T22:05:05Z

package/CHANGELOG

    enforced to be of the form ``[lx, ly, lz, alpha, beta, gamma]`` as required
    by the docs (Issue #2046, PR #2048)
  * Added periodic keyword to select_atoms (#759)
+  * PCA.transform now requires that PCA.run has already been called (PR #2055)


Add: PCA.transform now requires that run has been called before, otherwise aValueError is raised.

codecov · 2018-09-18T22:52:57Z

Codecov Report

Merging #2055 into develop will increase coverage by 0.04%.
The diff coverage is 90.62%.

@@             Coverage Diff             @@
##           develop    #2055      +/-   ##
===========================================
+ Coverage    89.33%   89.37%   +0.04%     
===========================================
  Files          159      159              
  Lines        18729    18712      -17     
  Branches      2684     2684              
===========================================
- Hits         16731    16724       -7     
+ Misses        1396     1384      -12     
- Partials       602      604       +2

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/contacts.py	`97.56% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/density.py	`64.94% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/helanal.py	`85.24% <100%> (-0.05%)`	⬇️
package/MDAnalysis/analysis/rms.py	`90.66% <100%> (-0.36%)`	⬇️
package/MDAnalysis/analysis/waterdynamics.py	`81.13% <100%> (+0.19%)`	⬆️
...ckage/MDAnalysis/analysis/hbonds/hbond_analysis.py	`80.64% <100%> (+0.18%)`	⬆️
package/MDAnalysis/analysis/diffusionmap.py	`90.62% <100%> (ø)`	⬆️
package/MDAnalysis/lib/log.py	`97.7% <100%> (-0.22%)`	⬇️
package/MDAnalysis/core/universe.py	`95.04% <100%> (-0.02%)`	⬇️
...age/MDAnalysis/analysis/hbonds/wbridge_analysis.py	`86.45% <71.42%> (+0.46%)`	⬆️
... and 6 more

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 259f778...21843d3. Read the comment docs.

codecov · 2018-09-18T22:53:44Z

Codecov Report

Merging #2055 into develop will increase coverage by 0.04%.
The diff coverage is 90.62%.

@@             Coverage Diff             @@
##           develop    #2055      +/-   ##
===========================================
+ Coverage    89.33%   89.37%   +0.04%     
===========================================
  Files          159      159              
  Lines        18729    18712      -17     
  Branches      2684     2684              
===========================================
- Hits         16731    16724       -7     
+ Misses        1396     1384      -12     
- Partials       602      604       +2

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/contacts.py	`97.56% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/density.py	`64.94% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/helanal.py	`85.24% <100%> (-0.05%)`	⬇️
package/MDAnalysis/analysis/rms.py	`90.66% <100%> (-0.36%)`	⬇️
package/MDAnalysis/analysis/waterdynamics.py	`81.13% <100%> (+0.19%)`	⬆️
...ckage/MDAnalysis/analysis/hbonds/hbond_analysis.py	`80.64% <100%> (+0.18%)`	⬆️
package/MDAnalysis/analysis/diffusionmap.py	`90.62% <100%> (ø)`	⬆️
package/MDAnalysis/lib/log.py	`97.7% <100%> (-0.22%)`	⬇️
package/MDAnalysis/core/universe.py	`95.04% <100%> (-0.02%)`	⬇️
...age/MDAnalysis/analysis/hbonds/wbridge_analysis.py	`86.45% <71.42%> (+0.46%)`	⬆️
... and 6 more

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 259f778...21843d3. Read the comment docs.

codecov · 2018-09-18T22:54:01Z

Codecov Report

Merging #2055 into develop will increase coverage by 0.04%.
The diff coverage is 90.62%.

@@             Coverage Diff             @@
##           develop    #2055      +/-   ##
===========================================
+ Coverage    89.33%   89.37%   +0.04%     
===========================================
  Files          159      159              
  Lines        18729    18712      -17     
  Branches      2684     2684              
===========================================
- Hits         16731    16724       -7     
+ Misses        1396     1384      -12     
- Partials       602      604       +2

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/contacts.py	`97.56% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/density.py	`64.94% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/helanal.py	`85.24% <100%> (-0.05%)`	⬇️
package/MDAnalysis/analysis/rms.py	`90.66% <100%> (-0.36%)`	⬇️
package/MDAnalysis/analysis/waterdynamics.py	`81.13% <100%> (+0.19%)`	⬆️
...ckage/MDAnalysis/analysis/hbonds/hbond_analysis.py	`80.64% <100%> (+0.18%)`	⬆️
package/MDAnalysis/analysis/diffusionmap.py	`90.62% <100%> (ø)`	⬆️
package/MDAnalysis/lib/log.py	`97.7% <100%> (-0.22%)`	⬇️
package/MDAnalysis/core/universe.py	`95.04% <100%> (-0.02%)`	⬇️
...age/MDAnalysis/analysis/hbonds/wbridge_analysis.py	`86.45% <71.42%> (+0.46%)`	⬆️
... and 6 more

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 259f778...21843d3. Read the comment docs.

codecov · 2018-09-18T22:54:10Z

Codecov Report

Merging #2055 into develop will increase coverage by 0.04%.
The diff coverage is 90.62%.

@@             Coverage Diff             @@
##           develop    #2055      +/-   ##
===========================================
+ Coverage    89.33%   89.37%   +0.04%     
===========================================
  Files          159      159              
  Lines        18729    18712      -17     
  Branches      2684     2684              
===========================================
- Hits         16731    16724       -7     
+ Misses        1396     1384      -12     
- Partials       602      604       +2

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/contacts.py	`97.56% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/density.py	`64.94% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/helanal.py	`85.24% <100%> (-0.05%)`	⬇️
package/MDAnalysis/analysis/rms.py	`90.66% <100%> (-0.36%)`	⬇️
package/MDAnalysis/analysis/waterdynamics.py	`81.13% <100%> (+0.19%)`	⬆️
...ckage/MDAnalysis/analysis/hbonds/hbond_analysis.py	`80.64% <100%> (+0.18%)`	⬆️
package/MDAnalysis/analysis/diffusionmap.py	`90.62% <100%> (ø)`	⬆️
package/MDAnalysis/lib/log.py	`97.7% <100%> (-0.22%)`	⬇️
package/MDAnalysis/core/universe.py	`95.04% <100%> (-0.02%)`	⬇️
...age/MDAnalysis/analysis/hbonds/wbridge_analysis.py	`86.45% <71.42%> (+0.46%)`	⬆️
... and 6 more

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 259f778...21843d3. Read the comment docs.

codecov · 2018-09-18T23:09:17Z

Codecov Report

Merging #2055 into develop will increase coverage by 0.01%.
The diff coverage is 90.62%.

@@             Coverage Diff             @@
##           develop    #2055      +/-   ##
===========================================
+ Coverage    89.35%   89.37%   +0.01%     
===========================================
  Files          159      159              
  Lines        18729    18712      -17     
  Branches      2684     2684              
===========================================
- Hits         16736    16724      -12     
+ Misses        1392     1384       -8     
- Partials       601      604       +3

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/contacts.py	`97.56% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/density.py	`64.94% <ø> (ø)`	⬆️
package/MDAnalysis/analysis/helanal.py	`85.24% <100%> (-0.05%)`	⬇️
package/MDAnalysis/analysis/rms.py	`90.66% <100%> (-0.36%)`	⬇️
package/MDAnalysis/analysis/waterdynamics.py	`81.13% <100%> (+0.19%)`	⬆️
...ckage/MDAnalysis/analysis/hbonds/hbond_analysis.py	`80.64% <100%> (+0.18%)`	⬆️
package/MDAnalysis/analysis/diffusionmap.py	`90.62% <100%> (ø)`	⬆️
package/MDAnalysis/lib/log.py	`97.7% <100%> (-0.22%)`	⬇️
package/MDAnalysis/core/universe.py	`95.04% <100%> (-0.02%)`	⬇️
...age/MDAnalysis/analysis/hbonds/wbridge_analysis.py	`86.45% <71.42%> (+0.46%)`	⬆️
... and 4 more

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 55e7475...00cf3b7. Read the comment docs.

orbeckst · 2018-09-20T00:54:39Z

@richardjgowers all that is needed are doc/CHANGELOG additions. Then you'll have fixed two issues! (<🥕 dangle/>)

richardjgowers · 2018-09-21T00:26:49Z

@orbeckst gepöken

…

On Wed, Sep 19, 2018 at 7:54 PM, Oliver Beckstein ***@***.***> wrote: @richardjgowers <https://github.com/richardjgowers> all that is needed are doc/CHANGELOG additions. Then you'll have fixed two issues! (<carrot dangle/>) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2055 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI0jB6bNgyN-FRRZMftifCY3Qtk9MDWxks5ucudQgaJpZM4WIO9r> .

updated usage of Analysis API, start/stop/step now deprecated in __init__ and should be given in run method made PCA setup respect start frame more, if start is given then initial weights are from that frame not frame 0

richardjgowers force-pushed the issue-1975-nomorequiet branch from 627eeff to 578014b Compare August 22, 2018 20:47

richardjgowers force-pushed the issue-1975-nomorequiet branch 2 times, most recently from cc9100c to 274b4e2 Compare August 31, 2018 15:17

richardjgowers force-pushed the issue-1975-nomorequiet branch from 274b4e2 to ffd1614 Compare September 11, 2018 19:16

orbeckst force-pushed the issue-1975-nomorequiet branch from cdf268f to 647343f Compare September 12, 2018 01:02

richardjgowers force-pushed the issue-1975-nomorequiet branch 2 times, most recently from 9a43259 to b92b502 Compare September 13, 2018 21:44

richardjgowers added this to the 0.19.0 milestone Sep 13, 2018

richardjgowers commented Sep 14, 2018

View reviewed changes

package/MDAnalysis/analysis/pca.py Show resolved Hide resolved

richardjgowers commented Sep 14, 2018

View reviewed changes

orbeckst reviewed Sep 14, 2018

View reviewed changes

orbeckst requested changes Sep 18, 2018

View reviewed changes

richardjgowers force-pushed the issue-1975-nomorequiet branch from 21843d3 to 2d34175 Compare September 20, 2018 16:58

removed quiet in favour of verbose

00cf3b7

updated usage of Analysis API, start/stop/step now deprecated in __init__ and should be given in run method made PCA setup respect start frame more, if start is given then initial weights are from that frame not frame 0

richardjgowers force-pushed the issue-1975-nomorequiet branch from 2d34175 to 00cf3b7 Compare September 21, 2018 15:25

orbeckst approved these changes Sep 21, 2018

View reviewed changes

orbeckst merged commit 8918d42 into develop Sep 21, 2018

orbeckst deleted the issue-1975-nomorequiet branch September 21, 2018 18:15

Conversation

richardjgowers commented Aug 22, 2018 • edited by orbeckst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes made in this Pull Request:

PR Checklist

Uh oh!

orbeckst commented Aug 31, 2018

Uh oh!

orbeckst commented Sep 11, 2018

Uh oh!

orbeckst commented Sep 12, 2018

Uh oh!

richardjgowers commented Sep 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdetle commented Sep 14, 2018 • edited by orbeckst Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 18, 2018

Codecov Report

Uh oh!

richardjgowers commented Sep 18, 2018

Uh oh!

orbeckst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 18, 2018

Codecov Report

Uh oh!

codecov bot commented Sep 18, 2018

Codecov Report

Uh oh!

codecov bot commented Sep 18, 2018

Codecov Report

Uh oh!

codecov bot commented Sep 18, 2018

Codecov Report

Uh oh!

codecov bot commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

orbeckst commented Sep 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardjgowers commented Sep 21, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

richardjgowers commented Aug 22, 2018 •

edited by orbeckst

Loading

jdetle commented Sep 14, 2018 •

edited by orbeckst

Loading

codecov bot commented Sep 18, 2018 •

edited

Loading

orbeckst commented Sep 20, 2018 •

edited

Loading