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

Data structures #14

Merged
merged 17 commits into from Oct 26, 2019
Merged

Data structures #14

merged 17 commits into from Oct 26, 2019

Conversation

@lilyminium
Copy link
Member

@lilyminium lilyminium commented Sep 29, 2019

Universe

  • Universe creation
    • Jupyter notebook for adding topology attributes
  • Explaining groups of atoms (residues, segments, fragments, molecules, residuegroups, segment groups)

AtomGroup

  • AtomGroup creation
  • Topology objects
  • Coordinate-related methods
doc/source/conf.py Outdated Show resolved Hide resolved
@lilyminium
Copy link
Member Author

@lilyminium lilyminium commented Oct 10, 2019

@orbeckst Do you have any thoughts on the current content of the data-structures page: primarily ways to create Universes and AtomGroups, plus links to the Topology section for actions involving:

  • adding Residues/Segments
  • TopologyObjects
  • TopologyAttrs

Copy link
Member

@orbeckst orbeckst left a comment

Reads very nicely and I like the logical grouping. It's a lot of content, perhaps some of these pages can be split into smaller pages.

I'd also try to use sphinx linking as much as possible: use ref instead of hard links, use intersphinx linking to numpy, Python, and the MDAnalysis docs (which should become the API docs, similar to what you have tested here).

See comments inline.

doc/source/data_structures.rst Outdated Show resolved Hide resolved
doc/source/data_structures.rst Outdated Show resolved Hide resolved
doc/source/data_structures.rst Outdated Show resolved Hide resolved
doc/source/data_structures.rst Outdated Show resolved Hide resolved
doc/source/data_structures.rst Outdated Show resolved Hide resolved
doc/source/selections.rst Outdated Show resolved Hide resolved
doc/source/topology_system.rst Outdated Show resolved Hide resolved
doc/source/topology_system.rst Show resolved Hide resolved
doc/source/topology_system.rst Outdated Show resolved Hide resolved
doc/source/topology_system.rst Show resolved Hide resolved
@lilyminium
Copy link
Member Author

@lilyminium lilyminium commented Oct 25, 2019

@orbeckst Unless you have any concerns I might merge this sometime late tomorrow (Australian) and ask the developer mailing list to comment.

- source activate testenv
- pip install --upgrade sphinx sphinx-sitemap nbsphinx sphinx_rtd_theme ipython
- jupyter-nbextension enable nglview --py --sys-prefix
script:
- make -C ${SPHINX_DIR} html && touch ${HTML_DIR}/.nojekyll
deploy:
Copy link
Member

@richardjgowers richardjgowers Oct 25, 2019

Choose a reason for hiding this comment

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

Kill the DS Store file below

Copy link
Member

@orbeckst orbeckst Oct 25, 2019

Choose a reason for hiding this comment

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

Add a top-level .gitignore ! You can copy the MDA .gitignore

doc/source/atomgroup.rst Show resolved Hide resolved
doc/source/groups_of_atoms.rst Show resolved Hide resolved
Copy link
Member

@orbeckst orbeckst left a comment

Excellent stuff... just some nit-picking.

Notes on the notebooks (viewed via nbviewer)

P.S.: Extra cool: I can manipulate the image in the nbviewer static notebook. So awesome!!!! How did you do that?

General

I have various comments regarding "moving advanced stuff into extra sections and using links/footnotes". You can certainly just go ahead and merge and then potentially address these later. It would be great to get this PR merged.

Do you have something on the first page that says which minimum version of MDAnalysis this guide applies to? Also include the release of the guide and the date. (See MDAnalysisTutorial – I find all of this information very useful in assessing the relevance.)

- source activate testenv
- pip install --upgrade sphinx sphinx-sitemap nbsphinx sphinx_rtd_theme ipython
- jupyter-nbextension enable nglview --py --sys-prefix
script:
- make -C ${SPHINX_DIR} html && touch ${HTML_DIR}/.nojekyll
deploy:
Copy link
Member

@orbeckst orbeckst Oct 25, 2019

Choose a reason for hiding this comment

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

Add a top-level .gitignore ! You can copy the MDA .gitignore

doc/source/groups_of_atoms.rst Outdated Show resolved Hide resolved
doc/source/groups_of_atoms.rst Outdated Show resolved Hide resolved
doc/source/groups_of_atoms.rst Outdated Show resolved Hide resolved
doc/source/groups_of_atoms.rst Outdated Show resolved Hide resolved

The below names are drawn from the CHARMM 27, OPLS-AA, GROMOS 53A6, AMBER 03, and AMBER 99sb*-ILDN force fields.
+------+------+------+------+------+------+------+------+
Copy link
Member

@orbeckst orbeckst Oct 25, 2019

Choose a reason for hiding this comment

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

Is there a way to generate this table programmatically from the MDAnalysis version that the UserGuide pertains to?

Although this is not likely to change soon, when it does I guarantee that someone will forget to update the UserGuide.

I think @jbarnoud has written code to, for instance, dynamically create the AUTHORS list in the docs.

Copy link
Member Author

@lilyminium lilyminium Oct 26, 2019

Choose a reason for hiding this comment

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

I think that sticking it in conf.py to run automatically would require installing MDAnalysis. If that isn't too heavy, this would be quite easy. Otherwise a script could be run manually from time-to-time to just regenerate standard_selections.rst?

Choose a reason for hiding this comment

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

At least, you can have the table in a separate file that gets included here. This way, you can generate the table with a script without having to deal with the rest of the page in the script. I have an example in MDAnalysis/mdanalysis#2176; it is a bit of a monster, but mostly because of what gets written in the external file.

doc/source/standard_selections.rst Show resolved Hide resolved
doc/source/standard_selections.rst Show resolved Hide resolved

MDAnalysis will ignore everything after the ``*``. ``u.select_atoms("resname *E")`` will not select atoms whose residue name ends in E, but instead select every atom.


Copy link
Member

@orbeckst orbeckst Oct 25, 2019

Choose a reason for hiding this comment

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

Is there a way that we only maintain this nice documentation of selections in a single place? If we update it in the code then we mustn't forget to update it here. (Not something to be solved in this PR but generally something to think about: How do we avoid duplicating docs that need to be updated.)

Trajectories
===============

MDAnalysis groups dynamic data about a :code:`Universe` into its trajectory. This is typically loaded from a trajectory file.
Copy link
Member

@orbeckst orbeckst Oct 25, 2019

Choose a reason for hiding this comment

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

TBD ;-)

Lily Wang added 2 commits Oct 26, 2019
* changed fragment example
* added advanced topology section
* now auto-generates standard selections
@lilyminium
Copy link
Member Author

@lilyminium lilyminium commented Oct 26, 2019

Thanks for the reviews @richardjgowers and @orbeckst, and for the pointer to the script @jbarnoud. I have now:

  • switched the KALP fragment example to lysozyme in water
  • updated the constructing universes notebook to have a correct water geometry and added a section on tiling
  • I hope removed .DS_Store?
  • added a script to generate files for the tables in standard_selections.rst

@orbeckst NGLView is cool! You can save the notebook widget state, and I think I had to include the javascript for widgets with this for it to render:

html_js_files = [
    'https://cdnjs.cloudflare.com/ajax/libs/require.js/2.3.4/require.min.js',
    DEFAULT_EMBED_REQUIREJS_URL,
]

(from this issue)

re: the MDAnalysis version -- I think ideally the UserGuide should apply to the most up-to-date one, with notes on when major changes happened? The tutorials should definitely have minimum versions on them. I haven't added them yet, but will once I work out which versions they work with.

@lilyminium lilyminium merged commit 48dd10c into master Oct 26, 2019
2 checks passed
@lilyminium lilyminium deleted the data-structures branch Oct 26, 2019
@lilyminium lilyminium mentioned this pull request Oct 26, 2019
7 tasks
@orbeckst
Copy link
Member

@orbeckst orbeckst commented Oct 29, 2019

Sorry, my comments are too late to the party – but they were all basically 👍 .

@lilyminium lilyminium mentioned this pull request Dec 28, 2019
5 tasks
@lilyminium lilyminium mentioned this pull request Dec 28, 2019
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants