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.

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

Fix #2582: polymer persistence length example fix #2589

Merged
merged 2 commits into from
Mar 7, 2020

Conversation

ss62171
Copy link
Contributor

@ss62171 ss62171 commented Mar 6, 2020

Fixes #2582

Changes made in this Pull Request:

lp variable is renamed to persistence length and missing * is added after H 'not name O* H'.

pl

PR Checklist

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

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.

  • Please add the issue number polymer persistence length example fails #2582 in the "Fix #" line of your issue so that PR and issue are linked.
  • Did you run the example? Can you please show some evidence that this works, e.g., run in a jupyter notebook and show a screenshot of the executed code and the resulting plot. (Normally we would have tests but the doc examples are not explicitly tested)

Thanks.

@codecov
Copy link

codecov bot commented Mar 6, 2020

Codecov Report

Merging #2589 into develop will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #2589   +/-   ##
========================================
  Coverage    90.57%   90.57%           
========================================
  Files          173      173           
  Lines        23383    23383           
  Branches      3038     3038           
========================================
  Hits         21180    21180           
  Misses        1585     1585           
  Partials       618      618           

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 1f4e492...c9da68a. Read the comment docs.

@ss62171
Copy link
Contributor Author

ss62171 commented Mar 6, 2020

@orbeckst is this fine?

@orbeckst
Copy link
Member

orbeckst commented Mar 6, 2020

This is looking good. Can you show the plot to?

plt.savefig("persistence.jpg")

and attach as a comment.

@ss62171
Copy link
Contributor Author

ss62171 commented Mar 6, 2020

Here is the plot.
persistence

@orbeckst
Copy link
Member

orbeckst commented Mar 6, 2020

Why are there multiple plots? Perhaps do plt.clf() before plotting.

@ss62171
Copy link
Contributor Author

ss62171 commented Mar 6, 2020

I didn't know about plt.clf before. Here is the plot
persistence

@orbeckst
Copy link
Member

orbeckst commented Mar 6, 2020

Looking good!

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Almost done. As you are new contributor to the code base:

  • add yourself to AUTHORS
  • add an entry "fixed example docs for polymer persistence length (polymer persistence length example fails #2582)" under Fixes to CHANGELOG (and don't forget to add your GitHub handle to author list for the next release)

@ss62171
Copy link
Contributor Author

ss62171 commented Mar 7, 2020

@orbeckst I couldn't figure out how to add my github handle to author list.

@ss62171 ss62171 force-pushed the persistence_length branch from c0956e7 to 6226217 Compare March 7, 2020 01:04
@orbeckst
Copy link
Member

orbeckst commented Mar 7, 2020

Have a look at what @richardjgowers did for you do you know how to do it in the future.

@ss62171
Copy link
Contributor Author

ss62171 commented Mar 7, 2020

Yes i do understand it

@orbeckst orbeckst merged commit 82e0d6d into MDAnalysis:develop Mar 7, 2020
@orbeckst
Copy link
Member

orbeckst commented Mar 7, 2020

@ss62171 congratulations on your first contribution! Thank you!

@ss62171
Copy link
Contributor Author

ss62171 commented Mar 7, 2020

It was really fun to work on this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

polymer persistence length example fails
5 participants