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

Update rms notebooks #75

Merged
merged 6 commits into from
Jul 29, 2020
Merged

Update rms notebooks #75

merged 6 commits into from
Jul 29, 2020

Conversation

lilyminium
Copy link
Member

@lilyminium lilyminium commented Jun 14, 2020

RMS notebook updated for 1.0:

  • added weights_groupselections
  • updated charge-weighting example (a bit meaningless as CAs all had same charge) to residue-number weighting, with emphasis that it's an example only

Built: https://mdauserguide.readthedocs.io/en/rms-1.0/

Notebooks:

@lilyminium lilyminium mentioned this pull request Jun 14, 2020
@lilyminium lilyminium added the version-1.0 Contains 1.0 features label Jun 15, 2020
@lilyminium lilyminium requested a review from IAlibay June 19, 2020 03:37
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of thoughts on what could be done to make it a little bit clearer.

@@ -108,12 +117,12 @@
"name": "stderr",
"output_type": "stream",
"text": [
"Step 98/98 [100.0%]\n"
"Step 102/102 [100.0%]\n"
Copy link
Member

Choose a reason for hiding this comment

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

locally this progress bar doesn't show up with the current code, you need to passs verbose=True to run(). Also as of v1.0 the progress bar should be tdqm not the old progress meter.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm ok with it not showing up unless you think it's necessary? Re-ran with 1.0 to get rid of the bar.

Copy link
Member

Choose a reason for hiding this comment

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

As long as we show off the progress bar somewhere it doesn't matter. That being said, maybe a "unified style" might be favourable, i.e. all progress bars or all no progress bars.

Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

Sorry, I completely forgot to review the second notebook.

doc/source/examples/analysis/alignment_and_rms/rmsd.ipynb Outdated Show resolved Hide resolved
doc/source/examples/analysis/alignment_and_rms/rmsd.ipynb Outdated Show resolved Hide resolved
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

The new background sections look really awesome!

Just a couple of comments & thoughts.

@lilyminium
Copy link
Member Author

Thanks for your comments @IAlibay and @orbeckst! I've just pushed some fixes and added the RMSF notebook. It probably won't build though due to #82.

@lilyminium
Copy link
Member Author

RTD is still segfaulting because it still installs MDAnalysis via conda (see MDAnalysis/mdanalysis#2818) so I've revived my fork.

Notebooks:

@lilyminium lilyminium requested a review from IAlibay July 8, 2020 07:07
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

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

LGTM!

Unfortunately can't comment directly on the diff for RMSF, here are some minor comments:

  • "If you use these datasets from MDAnalysisData, it may take some time to download." sounds a bit awkwardly worded for me, how about "Note: downloading these datasets from MDAnalysisData may take some time".

  • "where x is the coordinates" do we want to be a bit more specific here and say "of particle i" and something along the lines of "where <> indicates the ensemble average".

@IAlibay
Copy link
Member

IAlibay commented Jul 17, 2020

Note: just restarting the Travis build to test something out. I've been finding that the astropy Travis build has started failing elsewhere similarly to when we were using standard conda environments. Just checking it's not also an issue here.

Edit: well at least it's not the same issue, but it does fail :( https://travis-ci.com/github/MDAnalysis/UserGuide/builds/174744621

@lilyminium
Copy link
Member Author

*** Error in `/home/travis/miniconda/envs/test/bin/python3.6': free(): invalid pointer: 0x00007f65667bf620 ***

This happens after the pages are built and a sitemap is generated; what is Python even doing at that point?

@orbeckst
Copy link
Member

Garbage collecting and possibly running into a bug in the XDRReader??

@IAlibay
Copy link
Member

IAlibay commented Jul 18, 2020

Not getting the same issue, but it looks like the problem might be down to numpy==1.19.0. (see #95). It might be worth testing if pinning to 1.18.5 fixes the CI @lilyminium

@IAlibay IAlibay mentioned this pull request Jul 18, 2020
@lilyminium lilyminium changed the title Update rms notebook Update rms notebooks Jul 29, 2020
@lilyminium lilyminium merged commit 239b2d8 into MDAnalysis:master Jul 29, 2020
@lilyminium lilyminium deleted the rms-1.0 branch July 29, 2020 04:55
@orbeckst orbeckst mentioned this pull request Aug 8, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
version-1.0 Contains 1.0 features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants