Closed ojeda-e closed 3 years ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
Hmm. I don't see the review comments I left on the notebook, either here or ReviewNB, so... hope they got through. I was mostly asking for clarification on things like why you chose
wrap=True
and the number of bins -- imagine a new PhD student has been emailed your notebook from someone else, and has not been following along this project or been doing membrane research a long time :-)
Oh no. I didn't get any of the reviews. As you said, neither on ReviewNB nor here on git. Arf. Thanks for those points. I'll add them with the nbextension
added to the requirements. I completely missed adding that file in the push, silly me.
Regarding the NGLView visualization. Do you mean it doesn't even render for you in your browser, or that it doesn't render on ReviewNB? I don't believe ReviewNB will ever render it, as it doesn't support JavaScript; however, if you can get ReadTheDocs to build, you could view the widgets there.
I mean, I can see it on my notebook, but not on the rendered docs. Maybe this is what I missed:
After you have rendered the NGL widgets and see it in your notebook, you then need to click Widgets > Save Notebook Widget State for the widget to get saved into the notebook. That way Sphinx and ReadTheDocs will be able to make the interactive viewer for you.
Thanks @lilyminium !
Hello @ojeda-e! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:
I added comments at https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue63/docs/source/pages/Curvature_membrane-only_systems.ipynb/
EDIT: It's a bit weird that the comments in https://app.reviewnb.com/MDAnalysis/membrane-curvature/blob/issue63/docs%2Fsource%2Fpages%2FCurvature_membrane-only_systems.ipynb%2F/discussion are in reverse order so comments on the last part of the notebook are first.
@lilyminium please write a blog post with the "embedding nglview howto" https://github.com/MDAnalysis/membrane-curvature/pull/64#pullrequestreview-726642879 so that I can search it when I need it...
Having looked into that rotation state not persisting when the notebook is turned into HTML, I think it's just not being saved in the metadata. I've asked Hai about it here: https://github.com/nglviewer/nglview/issues/948#issuecomment-898121063
It's also not showing up in the ReadTheDocs check, though. You might need to either save your widget states again, or maybe add nglview
into the docs/requirements.yaml
?
Hai suggested using your mouse to drag it a little (or drag it out and back again). This triggers the widget into updating the rotation state. When I did that on the notebook, saved the widget state and ran make html
, the rotated view of the box rendered properly.
Thanks @orbeckst and @lilyminium for all your suggestions, comments and help with the widgets. It works locally as suggested in this comment, so thanks @lilyminium for keeping an eye on this!
I think the changes in the new version of the tutorial are substantial. Some of the changes include:
I just want to add a couple of comments here:
I ran a simulation with a bigger membrane patch and added a short traj to the test files to use in the tutorial.
From the previous review, it was suggested to discuss the meaning of the plots. I added the interpretation for the plots of surface, mean and gaussian for the first approach in visualization. However, when I offered a second option to plot using imshow
, I am not writing the interpretation again. If you consider it's better to add it, although repetitive, it can be fixed. :)
Although I manage to save the notebook with the widgets as suggested here, I'm still failing to get the widget to work properly. As you can see in the build docs, there is no widget yet. I am not sure why I get this behavior. Another strange difference is that the bullet points added in the notebook do not render for me locally, but they show up in the build. I get WARNING: nbsphinx_widgets_path not given and ipywidgets module unavailable looking for now-outdated files... none found
in the build. I tried adding these two lines* but it didn't work, the warning doesn't go away.
nbsphinx_requirejs_path = ""
nbsphinx_widgets_path = ""
Thanks
@ojeda-e does adding ipywidgets and nglview to requirements.yaml
solve the issue?
@ojeda-e does adding ipywidgets and nglview to
requirements.yaml
solve the issue?
Yes!, thanks!
Thanks @lilyminium for your help to make my widgets work in the tutorial! The first tutorial was updated and it's ready for review. Maybe @orbeckst can take a look as well :)
Thanks
View / edit / reply to this conversation on ReviewNB
lilyminium commented on 2021-08-19T03:33:55Z ----------------------------------------------------------------
Do these dimensions change over the trajectory? Even if they don't, maybe you should explicitly address this as it's important in setting the grid bounds of the analysis.
ojeda-e commented on 2021-08-19T16:29:06Z ----------------------------------------------------------------
Thanks, that's a good point. Added now :)
View / edit / reply to this conversation on ReviewNB
lilyminium commented on 2021-08-19T03:36:21Z ----------------------------------------------------------------
"the beads that represent lipid headgroups are under the PO4
name. Then, we can select the phospholipid head groups in our Universe with:"
May I suggest something more like "the beads we choose for our phospholipid headgroups are called PO4"? Just because there are other beads that may be considered headgroups, such as ammonium.
ojeda-e commented on 2021-08-19T16:29:30Z ----------------------------------------------------------------
Yes, you are 100% correct here. Thanks.
View / edit / reply to this conversation on ReviewNB
lilyminium commented on 2021-08-19T03:38:12Z ----------------------------------------------------------------
Thank you for elaborating on why you chose that number of bins, it's very helpful!
View / edit / reply to this conversation on ReviewNB
lilyminium commented on 2021-08-19T03:41:00Z ----------------------------------------------------------------
From this plots, we can identify a "bump" in the middle region of the membrane (blue coloured), which is reduced progressively towards the edges fo the simulation box.
Just a couple typos: "From this plots" -> "from these plots"
"fo" -> of"
View / edit / reply to this conversation on ReviewNB
lilyminium commented on 2021-08-19T03:41:01Z ----------------------------------------------------------------
Typo: "advatange" -> "advantage"
This is looking very nice! I very much like all the pictures and visualizations you've put in. You've put a lot of effort into making a beautiful tutorial that is also comprehensive for people who are new both to the specific package, and to the analysis concept itself.
I didn't have much time to go into detail, but I did find a couple of typos. I think I remember that you did not want to put tick labels on your color bars, but I do feel that it means the plots lack some context as to the degree of curvature shown.
I'll just approve for now to not be blocking.
This PR fixes #63
I would appreciate some feedback for this tutorial while I finish the other two notebooks. @lilyminium @orbeckst @richardjgowers @IAlibay
Thanks!
Edit: I forgot to mention I couldn't manage to make the NGLwidget render, so I ended up adding the png files of the widgets. Hope that works as an alternative.