Fix #2590: Created new axes for plot of persistence_length. #2591

ss62171 · 2020-03-07T00:52:08Z

Fixes #2590 : New axes are created if ax is not passed already.

Changes made in this Pull Request:

PR Checklist

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

codecov · 2020-03-07T01:59:27Z

Codecov Report

Merging #2591 into develop will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #2591      +/-   ##
===========================================
+ Coverage    91.00%   91.02%   +0.02%     
===========================================
  Files          174      174              
  Lines        23550    23617      +67     
  Branches      3083     3083              
===========================================
+ Hits         21431    21498      +67     
  Misses        1497     1497              
  Partials       622      622

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/polymer.py	`100.00% <100.00%> (ø)`
util.py	`88.22% <0.00%> (+0.08%)`	⬆️
coordinates/base.py	`94.95% <0.00%> (+0.28%)`	⬆️
auxiliary/base.py	`91.29% <0.00%> (+0.56%)`	⬆️

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 4102481...7c1f1f7. Read the comment docs.

lilyminium

Thanks @ss62171, looks good. Could you add a small test that it's not plotting on the current axes if ax is not provided, and update CHANGELOG?

ss62171 · 2020-03-08T16:36:37Z

small test like do i have to test on my jupyter notebook and then attach screenshot or some other way?

lilyminium · 2020-03-09T21:55:31Z

Sorry for the late reply @ss62171. Could you please add a test in test_polymer.py that checks that the returned axes is not the current axes if you have not passed it in?

ss62171 · 2020-03-09T22:15:27Z

@lilyminium is it test_polymer.py or test_persistencelength.py since I am not able to find test_polymer.py file

lilyminium · 2020-03-09T22:23:33Z

@ss62171 Yes it's test_persistencelength.py, my mistake.

ss62171 · 2020-03-09T23:04:37Z

@lilyminium is this fine?

testsuite/MDAnalysisTests/analysis/test_persistencelength.py

lilyminium · 2020-03-10T00:14:48Z

testsuite/MDAnalysisTests/analysis/test_persistencelength.py

+
+    def test_current_axes(self, p_run):
+        ax2 = p_run.plot()
+        fig, ax = plt.subplots()        


Thanks for changing this -- however, because you create this after you call .plot(), again, the test will always pass but for the wrong reasons. You want to test that .plot() does not get your current axes that you have already created before calling .plot().

ss62171 · 2020-03-12T22:12:20Z

@lilyminium I ran this test in my jupyter-notebook and it's working fine but as soon as i test this using pytest it's failing. Is there any build required for test or something else. Attached is screenshot of jupyter-notebook.

lilyminium

@ss62171 Your changes look great! Travis is failing due to an unrelated error -- I guess we'll need to fix that first.

package/CHANGELOG

orbeckst

Almost all good, just some small comments.

orbeckst · 2020-03-13T00:31:20Z

package/CHANGELOG

@@ -65,9 +65,11 @@ Fixes
    argument. The directives parsed into bonds, angles, impropers, and dihedrals now
    match TPRParser. (PR #2408)
  * Added parmed to setup.py
+  * PersistenceLength.plot() now grab new axes if current axes not provided (Issue #2590)


Put it under Enhancements, I wouldn't call it a "fix" – it improves the user experience but the problem did not lead to wrong results or crashes.

Also add your gitgub handle at the end of the author list for "0.21" (if you are not there already)

package/CHANGELOG

package/MDAnalysis/analysis/polymer.py

ss62171 · 2020-03-13T01:29:46Z

@ss62171 Your changes look great! Travis is failing due to an unrelated error -- I guess we'll need to fix that first.

@lilyminium any idea like how to resolve this since I'm assuming this needs to be done for every parameter passed to the function.

…persistence_axes

orbeckst · 2020-03-20T00:27:29Z

@ss62171 sorry for the long silence. Can you try merging the latest develop into this branch? I think this issue was fixed recently.

…persistence_axes

ss62171 · 2020-03-20T18:33:31Z

@orbeckst after merging my branch to latest develop it still fails the test but in my jupyter-notebook it's working fine.

ss62171 · 2020-03-20T18:53:29Z

off topic, am i late for draft proposal for GSOC?

orbeckst · 2020-03-20T18:57:10Z

Which test is failing?

By the way, editing comments is confusing: in my email I see a figure of a plot that clearly shows wrong behavior and then I come to the issue and see a screen shot from a notebook without a plot. Let's keep things less confusing: Just post a new issue.

If you feel you must edit an issue, cross out what you don't want, add text saying "EDIT" or "UPDATE" and write the new thing to keep context. Remember, that people interact with the issue tracker in many different ways and for many core developers it's through the email messages, not the web interface.

Right now I have no idea what the problem is. Can you please explain clearly?

orbeckst · 2020-03-20T18:57:49Z

off topic, am i late for draft proposal?

If it's off-topic then that's an excellent question for the mailing list.

UPDATE: see https://groups.google.com/d/msg/mdnalysis-devel/4_ke-xNO-bU/2xZE02lsAwAJ

ss62171 · 2020-03-20T19:12:12Z

now I have no idea what the problem is. Can you please explain clearly?

So basically PersistenceLength.plot() grabs the current axes instead of creating a new figure,so you can end up with it plotting over something else unintentionally like in this attached figure

Hence i created new axes if no axes are passed to PersistenceLength.Plot() and added test for same. This test passes on my local notebook as shown in attached pic but failed on pytest (due to some upgrade in it's version).

I merged my branch with latest develop branch but still same behavior is shown.

@ss62171 Your changes look great! Travis is failing due to an unrelated error -- I guess we'll need to fix that first.

As @lilyminium suggested this is somewhat unrelated error.Now I am stuck at how to resolve this failing of travis.

orbeckst · 2020-03-20T21:07:52Z

All tests passed 👍 so it does not look like as if the "unrelated test error" #2591 (comment) is still present.

orbeckst

You deleted CHANGELOG, please bring it back.

Please also address my remaining comment. Or if you think that my comment does not make sense, start a discussion.

If you haven't done so already:

add an entry to CHANGELOG
add yourself to AUTHORS

orbeckst · 2020-03-20T21:30:58Z

package/CHANGELOG

@@ -1,2226 +0,0 @@
-# -*- tab-width: 2; indent-tabs-mode: nil; coding: utf-8 -*-


You must have deleted CHANGELOG by accident. Bring it back!!!!

package/MDAnalysis/analysis/polymer.py

ss62171 · 2020-03-20T21:56:03Z

Please also address my remaining comment. Or if you think that my comment does not make sense, start a discussion.

Yeah that 'unrelated error' is resolved however I added a test that is failing via pytest. But testing this using jupyter-notebook it seems to work. Here I am attaching pics of pytest output and my local jupyter-notebook for reference.

I hope this clears my issue :)

orbeckst

CHANGELOG got messed up, see comments. Please restore it. Check the diff to develop (e.g., looking at Files changed – you should only see the changes that directly relate to your PR.)

orbeckst · 2020-03-20T21:52:33Z

package/CHANGELOG

@@ -16,21 +16,11 @@ The rules for this file:
 mm/dd/yy richardjgowers, kain88-de, lilyminium, p-j-smith, bdice, joaomcteixeira,
         PicoCentauri, davidercruz, jbarnoud, RMeli, IAlibay, mtiberti, CCook96,
         Yuan-Yu, xiki-tempula, HTian1997, Iv-Hristov, hmacdope, AnshulAngaria,
-         ss62171, Luthaf, yuxuanzhuang, abhishandy, mlnance, shfrz, orbeckst,
-         wvandertoorn


do not delete lines from CHANGELOG

package/CHANGELOG

orbeckst · 2020-03-20T22:02:44Z

Your test appears to be passing on Travis and that's the primary concern when we run the tests because we know that the environment is set up correctly on Travis (and AppVeyor). Maybe you're not using a developer installation or you didn't reinstall your updated code.

What you're showing indicates that you pick up the correct code in your Jupyter Notebook but not when you run pytest. Perhaps pytest isn't installed in the same virtual environment... whatever it is, follow the User Guide instructions. I am also not sure if you should be running pytest inside the package/ directory. This could be a problem.

ss62171 · 2020-03-20T22:32:41Z

Tests passed using pytest.
pytest env was not same. Phew, I was stuck at this for last 2 days, thanks :)

ss62171 · 2020-03-20T22:35:04Z

@orbeckst can I import CHANGELOG file from develop branch to my branch?

lilyminium · 2020-03-20T23:38:16Z

@ss62171 that'll work if your develop branch is up-to-date -- otherwise you might need to rebase.

lilyminium

Looks good now @ss62171, thanks! Please add your GitHub username to CHANGELOG and your name to AUTHORS.

ss62171 · 2020-03-21T15:22:50Z

Looks good now @ss62171, thanks! Please add your GitHub username to CHANGELOG and your name to AUTHORS.

@lilyminium already done.

orbeckst

I don’t see your username in CHANGELOG and AUTHORS. Everything else looks good.

ss62171 · 2020-03-21T16:25:05Z

@orbeckst this is my 2nd PR, i already added my name in AUTHORS and username in CHANGELOG during my 1st PR. Do i need to add it again?

orbeckst · 2020-03-21T20:41:00Z

@ss62171 , apologies, I must have overlooked your previous additions to CHANGELOG and AUTHORS – of course you'll only have to add these once. I approved the PR. @lilyminium will do the rest.

lilyminium · 2020-03-21T23:10:52Z

Oops, I missed your earlier contribution @ss62171. Thanks for the fix!

Fix MDAnalysis#2590: Created new axes for plot of persistence_length.

dfc53fd

lilyminium requested changes Mar 8, 2020

View reviewed changes

IAlibay added the GSOC Starter label Mar 9, 2020

Added test for checking axes and CHANGELOGS updated

efb301c

lilyminium requested changes Mar 9, 2020

View reviewed changes

testsuite/MDAnalysisTests/analysis/test_persistencelength.py Outdated Show resolved Hide resolved

test updated

6594e76

ss62171 force-pushed the persistence_axes branch from da4772e to 6594e76 Compare March 9, 2020 23:50

lilyminium requested changes Mar 10, 2020

View reviewed changes

orbeckst assigned lilyminium Mar 10, 2020

test_updated

b31498d

ss62171 force-pushed the persistence_axes branch from bb29e09 to b31498d Compare March 12, 2020 22:06

lilyminium requested changes Mar 13, 2020

View reviewed changes

package/CHANGELOG Outdated Show resolved Hide resolved

orbeckst requested changes Mar 13, 2020

View reviewed changes

Some nitpick changes

05950ca

Merge branch 'develop' of https://github.com/ss62171/mdanalysis into …

71b26b5

…persistence_axes

ss62171 force-pushed the persistence_axes branch 2 times, most recently from 71b26b5 to 05950ca Compare March 13, 2020 01:42

Merge branch 'develop' of https://github.com/ss62171/mdanalysis into …

ea17ce3

…persistence_axes

ss62171 force-pushed the persistence_axes branch from f4ee126 to ea17ce3 Compare March 20, 2020 18:20

orbeckst requested changes Mar 20, 2020

View reviewed changes

polymer.py updated

d02e666

orbeckst requested changes Mar 20, 2020

View reviewed changes

CHANGELOG updated

7c1f1f7

lilyminium requested changes Mar 21, 2020

View reviewed changes

orbeckst requested changes Mar 21, 2020

View reviewed changes

orbeckst approved these changes Mar 21, 2020

View reviewed changes

lilyminium approved these changes Mar 21, 2020

View reviewed changes

lilyminium merged commit 9a0a657 into MDAnalysis:develop Mar 21, 2020

ss62171 deleted the persistence_axes branch March 21, 2020 23:26

fiona-naughton added defect Component-Analysis labels Sep 26, 2023

		@@ -1,2226 +0,0 @@
		# -- tab-width: 2; indent-tabs-mode: nil; coding: utf-8 --

Fix #2590: Created new axes for plot of persistence_length. #2591

Fix #2590: Created new axes for plot of persistence_length. #2591

Conversation

ss62171 commented Mar 7, 2020 • edited by orbeckst Loading

Changes made in this Pull Request:

PR Checklist

codecov bot commented Mar 7, 2020 • edited Loading

Codecov Report

lilyminium left a comment

Choose a reason for hiding this comment

ss62171 commented Mar 8, 2020

lilyminium commented Mar 9, 2020

ss62171 commented Mar 9, 2020

lilyminium commented Mar 9, 2020

ss62171 commented Mar 9, 2020

lilyminium Mar 10, 2020

Choose a reason for hiding this comment

ss62171 commented Mar 12, 2020

lilyminium left a comment

Choose a reason for hiding this comment

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst Mar 13, 2020

Choose a reason for hiding this comment

orbeckst Mar 13, 2020

Choose a reason for hiding this comment

ss62171 commented Mar 13, 2020

orbeckst commented Mar 20, 2020

ss62171 commented Mar 20, 2020 • edited Loading

ss62171 commented Mar 20, 2020 • edited Loading

orbeckst commented Mar 20, 2020

orbeckst commented Mar 20, 2020 • edited Loading

ss62171 commented Mar 20, 2020

orbeckst commented Mar 20, 2020

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst Mar 20, 2020

Choose a reason for hiding this comment

ss62171 commented Mar 20, 2020

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst Mar 20, 2020

Choose a reason for hiding this comment

orbeckst commented Mar 20, 2020

ss62171 commented Mar 20, 2020

ss62171 commented Mar 20, 2020

lilyminium commented Mar 20, 2020

lilyminium left a comment

Choose a reason for hiding this comment

ss62171 commented Mar 21, 2020

orbeckst left a comment

Choose a reason for hiding this comment

ss62171 commented Mar 21, 2020

orbeckst commented Mar 21, 2020

lilyminium commented Mar 21, 2020

ss62171 commented Mar 7, 2020 •

edited by orbeckst

Loading

codecov bot commented Mar 7, 2020 •

edited

Loading

ss62171 commented Mar 20, 2020 •

edited

Loading

ss62171 commented Mar 20, 2020 •

edited

Loading

orbeckst commented Mar 20, 2020 •

edited

Loading