-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
Hi @lilyminium @orbeckst. I would appreciate some feedback on the pushed changes. Thanks! |
There was a problem hiding this 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`.
The most recent pushed changes include:
Two points to discuss:
Edit: Thanks! |
I think this PR is ready for review.
Thanks @lilyminium for all the suggestions! |
There was a problem hiding this 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!
Thanks for taking the time to double-check the changes and approve the PR @lilyminium! |
Changes in this PR fixes #58
Changes include:
Pending:
Question:
I left the usage page as if I were going to use a different test data file for each case.
Happy to receive any other suggestions. Thanks!
@lilyminium @orbeckst @IAlibay @fiona-naughton