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

Updated docs pages #62

Merged
merged 10 commits into from Aug 8, 2021
Merged

Updated docs pages #62

merged 10 commits into from Aug 8, 2021

Conversation

ojeda-e
Copy link
Member

@ojeda-e ojeda-e commented Aug 4, 2021

Changes in this PR fixes #58

Changes include:

  • Updated algorithm page.
  • Update Usage page for three cases
  • Membrane-only
  • Membrane-protein with posres.
  • Membrane-protein with no posres.
  • Updated visualization page.

Pending:

  • Add real data file to the test suite.
  • Add real plots to the Visualization page.

Question:
I left the usage page as if I were going to use a different test data file for each case.

  • Is it a good idea to add a dataset for each one of the cases I have or better to reuse some MDA tests?

Happy to receive any other suggestions. Thanks!

@lilyminium @orbeckst @IAlibay @fiona-naughton

@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #62 (3e235e7) into main (8a52295) will not change coverage.
The diff coverage is n/a.

@ojeda-e
Copy link
Member Author

ojeda-e commented Aug 6, 2021

Hi @lilyminium @orbeckst. I would appreciate some feedback on the pushed changes.

Thanks!

Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

This looks amazing @ojeda-e! Lovely figures as always. I have some comments, mostly on how you can get sphinx to do the work for you.

Regarding test files, I do think it'd be a good idea to add the additional test files that you mention in the examples somewhere so users can immediately use your library, and you can also use them for tests. However, one option that you could consider (if they're small) is not adding them to MDAnalysis.tests, but rather adding them to membrane_curvature.tests. (If the files are large, they will be better off in MDAnalysis.data`.

docs/source/pages/Algorithm.rst Outdated Show resolved Hide resolved
docs/source/pages/Algorithm.rst Outdated Show resolved Hide resolved
docs/source/pages/Algorithm.rst Outdated Show resolved Hide resolved
docs/source/pages/Algorithm.rst Show resolved Hide resolved
docs/source/pages/Algorithm.rst Outdated Show resolved Hide resolved
docs/source/pages/Visualization.rst Outdated Show resolved Hide resolved
docs/source/pages/Visualization.rst Outdated Show resolved Hide resolved
docs/source/pages/Visualization.rst Outdated Show resolved Hide resolved
docs/source/pages/Visualization.rst Outdated Show resolved Hide resolved
docs/source/pages/Algorithm.rst Outdated Show resolved Hide resolved
@pep8speaks
Copy link

pep8speaks commented Aug 7, 2021

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! 🍻

Comment last updated at 2021-08-08 04:05:59 UTC

@ojeda-e
Copy link
Member Author

ojeda-e commented Aug 7, 2021

The most recent pushed changes include:

  • added ipython to the Visualization page to generate the plots. (Thanks @lilyminium for the suggestion, it looks amazing!)
  • added intersphinx_mapping to link external docs.
  • general requested changes.

Two points to discuss:

  • I couldn't fix the links of MembraneCurvature.results.avearge_z_surface and other attributes on the Algorithm page. Can I just leave it like that for now? I knwo it isn't perfect, but I tried several options and couldn't make it work as it should.

Edit: I am not sure what to do about the example for the case of protein with no posres. I can't make my systems public at this very moment. Maybe there is a system with such features in tests or MDAnalysisData? I'm open to suggestions case covered :) .

Thanks!

@ojeda-e
Copy link
Member Author

ojeda-e commented Aug 7, 2021

I think this PR is ready for review.

  • All usage cases are included.
  • In visualization, plots render with iPython.

Thanks @lilyminium for all the suggestions!
@orbeckst @IAlibay @fiona-naughton I would appreciate your feedback here although I think it's ready to go. :)

Copy link
Member

@lilyminium lilyminium 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 making those changes @ojeda-e!

@ojeda-e
Copy link
Member Author

ojeda-e commented Aug 8, 2021

Thanks for taking the time to double-check the changes and approve the PR @lilyminium!
I will merge after fixing the PEP8.

@ojeda-e ojeda-e merged commit 71aecab into main Aug 8, 2021
@ojeda-e ojeda-e deleted the issue58 branch August 29, 2021 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incomplete documentation 2/3
3 participants