DOCS: Align.py changes made #4182

MohitKumar020291 · 2023-06-29T05:50:11Z

Fixes #3365

Changes made in this Pull Request: This PR is resolving suggestions by @orbeckst in #4023.

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

📚 Documentation preview 📚: https://mdanalysis--4182.org.readthedocs.build/en/4182/

github-actions · 2023-06-29T05:52:55Z

Linter Bot Results:

Hi @MohitKumar020291! Thanks for making this PR. We linted your code and found the following:

Some issues were found with the formatting of your code.

Code Location	Outcome
main package	⚠️ Possible failure
testsuite	✅ Passed

Please have a look at the darker-main-code and darker-test-code steps here for more details: https://github.com/MDAnalysis/mdanalysis/actions/runs/5441610741/jobs/9895771696

Please note: The black linter is purely informational, you can safely ignore these outcomes if there are no flake8 failures!

codecov · 2023-06-29T06:04:22Z

Codecov Report

Patch coverage has no change and project coverage change: -0.30 ⚠️

Comparison is base (6532f7b) 93.61% compared to head (85b78ec) 93.32%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4182      +/-   ##
===========================================
- Coverage    93.61%   93.32%   -0.30%     
===========================================
  Files          193      193              
  Lines        25170    25170              
  Branches      4059     4059              
===========================================
- Hits         23562    23489      -73     
- Misses        1092     1158      +66     
- Partials       516      523       +7

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/align.py	`96.38% <ø> (ø)`

... and 15 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

orbeckst

Does your solution work?

Have you checked that your solution works? Show a screen shot when you run your code in a Python session.

Have you run the doc tests? According to #3925 you can run doc tests with the commands

cd package/doc/sphinx
make doctest

or alternatively follow #760 (comment) and invoke via pytest

cd package/MDAnalysis
pytest . --disable-pytest-warnings --doctest-modules

Does the test pass now? Show evidence: Show that you don't have the line

FAILED analysis/align.py::MDAnalysis.analysis.align.rotation_matrix

anymore.

New author

Add yourself to the file AUTHORS.
Send an email to the developer list introducing yourself (and mention this PR); if you've already done so, add a link to the email in the list archive in the comments of this PR.

package/MDAnalysis/analysis/align.py

orbeckst · 2023-07-01T05:38:25Z

The AUTHORS file is package/AUTHORS.

The error that you encounter may be due to not having installed MDAnalysis.

I suggest you follow the instructions in the user guide to create a development environment with conda https://userguide.mdanalysis.org/stable/contributing_code.html#creating-a-development-environment and then install a development version with pip install -e ./package.

If it still fails, describe step by step what you did. (Copy and paste code into the issue, that’s better than images; if only images are possible then you can directly drop them here, too)

MohitKumar020291 · 2023-07-01T06:19:15Z

@orbeckst thanks for review. I tried multiple ways of creating universe but doctests failed. I am attaching the screenshots, please help to solve them.
https://i.imgur.com/xCEdQbE.png
https://i.imgur.com/XRbeVKl.png

RMeli · 2023-07-01T09:07:03Z

@MohitKumar020291 the second screenshot corresponds to the issue outlined by @orbeckst. In the first screenshot I think the issue is that you are using TPR and TRR within strings, while they should be passed directly.

MohitKumar020291 · 2023-07-01T10:20:09Z

Thanks @RMeli for review. I tried passing TRR and TPR directly. But the results are not desirable. Screenshot attached.
https://i.imgur.com/U064e9k.png
EDIT: Please ignore, it is fixed now.

MohitKumar020291 · 2023-07-01T14:23:25Z

@RMeli and @orbeckst. I have made some latest change. I imported align as to use rotation_matrix. Secondly, I skipped one line because it was causing wrong output, Attached screenshots:

RELATED TO ABOVE IMAGE

After doing these changes I am not getting Failed test(like FAILED analysis\align.py::MDAnalysis.analysis.align.rotation_matrix ), Screenshot attached(THIS IMAGE DOES NOT HAVE Failing align.rotation_matrix as of SECOND IMAGE ):

This solves one out of two problems of align.py.

orbeckst

Let's avoid skipping and make the doc test fully work (see inline comment).

Note I can quickly run the doc test with

cd package/MDAnalysis
pytest -v --disable-pytest-warnings --doctest-modules analysis/align.py

I suggest you remove the # doctest: +SKIP, then run the above command. It will immediately tell you all you need to know to fix the problem.

Please also add yourself to AUTHORS and introduce yourself on the mailing list (see my previous review comments) — we won't merge a new contribution without the authorship/email information.

package/MDAnalysis/analysis/align.py

orbeckst · 2023-07-01T22:46:26Z

This solves one out of two problems of align.py.

Great that you identified another problem in the same file. It would be great if all tests in align.py were to pass — please, feel free to also fix this in the same PR here! It looks as if it's just a missing closing parenthesis.

=================================== FAILURES ===================================
_____________________ [doctest] MDAnalysis.analysis.align ______________________
083 mass (or geometry) first:
084
085    >>> rmsd(mobile.select_atoms('name CA').positions, ref.select_atoms('name CA').positions, center=True)
086    21.892591663632704
087
088 This has only done a translational superposition. If you want to also do a
089 rotational superposition use the superposition keyword. This will calculate a
090 minimized RMSD between the reference and mobile structure.
091
092    >>> rmsd(mobile.select_atoms('name CA').positions, ref.select_atoms('name CA').positions,
UNEXPECTED EXCEPTION: SyntaxError("'(' was never closed", ('<doctest MDAnalysis.analysis.align[8]>', 1, 5, "rmsd(mobile.select_atoms('name CA').positions, ref.select_atoms('name CA').positions,\n", 1, 0))
Traceback (most recent call last):
  File "~/anaconda3/envs/mda310dev/lib/python3.10/doctest.py", line 1348, in __run
    exec(compile(example.source, filename, "single",
  File "<doctest MDAnalysis.analysis.align[8]>", line 1
    rmsd(mobile.select_atoms('name CA').positions, ref.select_atoms('name CA').positions,

SyntaxError: '(' was never closed

orbeckst · 2023-07-01T23:06:32Z

I had a quick look at align.py: there are a few more errors. The "missing parenthesis" is only the first one (and it's not really a missing parenthesis, just an incorrect line continuation).

If you want to, you can quickly work through all of them and make this a very nice PR:

First run the doc test

pytest -v --disable-pytest-warnings --doctest-modules package/MDAnalysis/analysis/align.py

just on the file.

Then read the error message.
Fix the line.
Repeat with 1

At the end there's an example that uses the clustalw program: just skip all of it with

   >>> seldict = align.fasta2select('sequences.aln')                                # doctest:+SKIP
   >>> alignment = align.AlignTraj(trj, ref, filename='rmsfit.dcd', select=seldict) # doctest:+SKIP
   >>> alignment.run()                                                              # doctest:+SKIP

as this example is difficult to run under all circumstances (because clustalw is not always installed) and we don't have all input files at hand.

MohitKumar020291 · 2023-07-02T07:30:26Z

@orbeckst and @RMeli. I have again updated align.py and these changes helped to Pass both failures of align.py.
Screenshot attached:

Please review.

MohitKumar020291 · 2023-07-02T09:11:10Z

Please also add yourself to AUTHORS and introduce yourself on the mailing list (see my previous review comments) — we won't merge a new contribution without the authorship/email information.

I tried finding "how to add myself to changelog and author?", but did not found helpful results. So, I tried copying format in AUTHOR but I can't see 2023 in package/AUTHORS, so, I created my one(2023) which is obviously causing errors. Any solution to it?

orbeckst

Please fix the one test that I mentioned in the comments.

The AUTHORS file can be found in the checked out sources as package/AUTHORS. When you are in the top level directory that you got from git clone you should be able to cd packages and then edit the AUTHORS file.

package/MDAnalysis/analysis/align.py

package/CHANGELOG

orbeckst · 2023-07-02T09:30:03Z

I am trying to understand why it's difficult for you to find the AUTHORS file. How do you edit the files that you are changing?

MohitKumar020291 · 2023-07-02T09:55:36Z

I am trying to understand why it's difficult for you to find the AUTHORS file. How do you edit the files that you are changing?

@orbeckst I can find the AUTHORS file but not 2023 it has only 2022. I was trying to change it through edit(it is a bad practice) but it is also not showing in my VS code(on both develop and 3365 branch).

Apart from that I have resolved the latest changes.
Thanks for review

RMeli · 2023-07-02T10:39:26Z

@MohitKumar020291 you might have an older version of the repository and the errors you see are conflicts?

MohitKumar020291 · 2023-07-02T10:55:05Z

@RMeli I have latest version even then I will check it again. Yes, they were conflicts.

orbeckst · 2023-07-02T18:46:00Z

THanks for introducing yourself on the developer list.

The current AUTHORS file is on your branch: I can see it in the web interface https://github.com/MohitKumar020291/mdanalysis/blob/3365/package/AUTHORS. Maybe you have to git pull your local branch in order to update it? If there are merge conflicts then you have to fix them.

EDIT: The AUTHORS addition is the only thing that's missing before we can approve the PR and squash-merge.

MohitKumar020291 · 2023-07-03T01:14:19Z

@orbeckst I have successfully updated AUTHORS file. Thanks.

orbeckst

Thanks for adding yourself. I looked at your solution again. ALthough it works, it can be written in a more succinct manner. In order to keep the example code as simple as possible, I'd like you to change your code. I made suggestions that you should be able to simply accept and include. Thanks!

package/MDAnalysis/analysis/align.py

Co-authored-by: Oliver Beckstein <[email protected]>

MohitKumar020291 · 2023-07-03T02:38:06Z

@orbeckst changes done. Thanks.

orbeckst

Looking good! Thank you for your contribution.

I'll just wait until the CI comes back all green and then squash-merge.

orbeckst

There's now an issue with actually building the documentation, see https://github.com/MDAnalysis/mdanalysis/actions/runs/5439978586/jobs/9892463278?pr=4182#step:5:3019 . There's something wrong with the formatting. Please investigate. The build docs check must also pass.

It is possible that you need to indent the code blocks under ::.

Literal block issue

skip

MohitKumar020291 · 2023-07-03T06:30:25Z

@orbeckst I tried finding issues multiple times with formatting but I did not get anything.

orbeckst · 2023-07-03T07:34:42Z

I'd recommend that you

build the docs locally
check if it fails
fix file/try out things
goto 1 (until no errors)
commit & push
check CI

instead of having to wait for a while until the CI on GitHub runs.

To build the docs locally have a look at our User Guide: Building docs. You will need to set up a development environment in which you can build docs. Follow User Guide: Create a virtual environment (I would use conda/mamba). Unfortunately, the docs are a bit outdated. MDAnalysis/UserGuide#260 contains some instructions to be added to an updated User Guide version. In short, try in your conda environment (with python>=3.8):

# packages for MDAnalysis
conda install -c bioconda -c conda-forge \
            biopython chemfiles clustalw==2.1 codecov cython \
            griddataformats gsd hypothesis "joblib>=0.12" \
            matplotlib mmtf-python mock netcdf4 networkx \
            "numpy>=1.18.0" psutil pytest scikit-learn scipy \
            "seaborn>=0.7.0" "tidynamics>=1.0.0" \
            "tqdm>=4.43.0"

# documentation dependencies
conda install -c conda-forge sphinx pybtex pybtex-docutils \
	   sphinxcontrib-bibtex sphinx_rtd_theme sphinx-sitemap
python -m pip install msmb_theme==1.2.0

Then you should be able to build the docs as described, in short

cd package
python setup.py build_sphinx -E

Look at the output to see where files are generated. Read the output for errors.

MohitKumar020291 · 2023-07-03T07:43:18Z

@orbeckst I think I have fixed the issue build docs, this is failing because of code coverage. Can you please rerun the checks?

orbeckst

Looks good! 🎉

The codecov thing is an annoying spurious failure and not related to your PR.

orbeckst · 2023-07-03T08:09:46Z

Well done! Thank you for your contribution!

MohitKumar020291 · 2023-07-03T08:27:12Z

@orbeckst and @RMeli, Thanks to both for reviews and great guidance.

Align.py

fe3bb12

github-actions bot added the Component-Analysis label Jun 29, 2023

MohitKumar020291 changed the title ~~DOC: Align.py changes made~~ DOCS: Align.py changes made Jun 29, 2023

MohitKumar020291 mentioned this pull request Jun 30, 2023

Failed doc examples in align.py #3365

Closed

orbeckst requested changes Jun 30, 2023

View reviewed changes

package/MDAnalysis/analysis/align.py Outdated Show resolved Hide resolved

orbeckst self-assigned this Jun 30, 2023

Update align.py

4b7e9fd

orbeckst requested changes Jul 1, 2023

View reviewed changes

package/MDAnalysis/analysis/align.py Outdated Show resolved Hide resolved

Update align.py

3516f2e

MohitKumar020291 added 4 commits July 2, 2023 13:33

Update AUTHORS

847eb5b

Update AUTHORS

2a2caec

Update AUTHORS

d41d38d

Update CHANGELOG

3925b0e

Merge branch 'develop' into 3365

0a4bf20

orbeckst requested changes Jul 2, 2023

View reviewed changes

package/MDAnalysis/analysis/align.py Outdated Show resolved Hide resolved

package/CHANGELOG Show resolved Hide resolved

Update align.py

baefd3d

Update AUTHORS

33609df

orbeckst requested changes Jul 3, 2023

View reviewed changes

package/MDAnalysis/analysis/align.py Outdated Show resolved Hide resolved

package/MDAnalysis/analysis/align.py Outdated Show resolved Hide resolved

MohitKumar020291 and others added 2 commits July 3, 2023 08:06

Update package/MDAnalysis/analysis/align.py

66ddafc

Co-authored-by: Oliver Beckstein <[email protected]>

Update package/MDAnalysis/analysis/align.py

3257d08

Co-authored-by: Oliver Beckstein <[email protected]>

orbeckst approved these changes Jul 3, 2023

View reviewed changes

orbeckst self-requested a review July 3, 2023 04:38

orbeckst requested changes Jul 3, 2023

View reviewed changes

MohitKumar020291 added 5 commits July 3, 2023 10:41

Update align.py

7b0378e

Literal block issue

Update align.py

3cdef3e

Update align.py

59552d8

skip

Update align.py

164bad1

Update align.py

0288406

MohitKumar020291 added 3 commits July 3, 2023 12:11

Update align.py

17b0dfa

Update align.py

6432605

Update align.py

85b78ec

orbeckst approved these changes Jul 3, 2023

View reviewed changes

orbeckst merged commit 69a00ad into MDAnalysis:develop Jul 3, 2023

IAlibay added the Component-Docs label Sep 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DOCS: Align.py changes made #4182

DOCS: Align.py changes made #4182

MohitKumar020291 commented Jun 29, 2023 •

edited

Loading

github-actions bot commented Jun 29, 2023 •

edited

Loading

codecov bot commented Jun 29, 2023 •

edited

Loading

orbeckst left a comment •

edited

Loading

orbeckst commented Jul 1, 2023

MohitKumar020291 commented Jul 1, 2023

RMeli commented Jul 1, 2023

MohitKumar020291 commented Jul 1, 2023 •

edited

Loading

MohitKumar020291 commented Jul 1, 2023 •

edited

Loading

orbeckst left a comment

orbeckst commented Jul 1, 2023

orbeckst commented Jul 1, 2023

MohitKumar020291 commented Jul 2, 2023 •

edited

Loading

MohitKumar020291 commented Jul 2, 2023 •

edited

Loading

orbeckst left a comment

orbeckst commented Jul 2, 2023

MohitKumar020291 commented Jul 2, 2023

RMeli commented Jul 2, 2023

MohitKumar020291 commented Jul 2, 2023 •

edited

Loading

orbeckst commented Jul 2, 2023 •

edited

Loading

MohitKumar020291 commented Jul 3, 2023

orbeckst left a comment

MohitKumar020291 commented Jul 3, 2023

orbeckst left a comment

orbeckst left a comment •

edited

Loading

MohitKumar020291 commented Jul 3, 2023 •

edited

Loading

orbeckst commented Jul 3, 2023

MohitKumar020291 commented Jul 3, 2023 •

edited

Loading

orbeckst left a comment

orbeckst commented Jul 3, 2023

MohitKumar020291 commented Jul 3, 2023

DOCS: Align.py changes made #4182

DOCS: Align.py changes made #4182

Conversation

MohitKumar020291 commented Jun 29, 2023 • edited Loading

PR Checklist

github-actions bot commented Jun 29, 2023 • edited Loading

Linter Bot Results:

codecov bot commented Jun 29, 2023 • edited Loading

Codecov Report

orbeckst left a comment • edited Loading

Choose a reason for hiding this comment

Does your solution work?

New author

orbeckst commented Jul 1, 2023

MohitKumar020291 commented Jul 1, 2023

RMeli commented Jul 1, 2023

MohitKumar020291 commented Jul 1, 2023 • edited Loading

MohitKumar020291 commented Jul 1, 2023 • edited Loading

RELATED TO ABOVE IMAGE

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Jul 1, 2023

orbeckst commented Jul 1, 2023

MohitKumar020291 commented Jul 2, 2023 • edited Loading

MohitKumar020291 commented Jul 2, 2023 • edited Loading

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Jul 2, 2023

MohitKumar020291 commented Jul 2, 2023

RMeli commented Jul 2, 2023

MohitKumar020291 commented Jul 2, 2023 • edited Loading

orbeckst commented Jul 2, 2023 • edited Loading

MohitKumar020291 commented Jul 3, 2023

orbeckst left a comment

Choose a reason for hiding this comment

MohitKumar020291 commented Jul 3, 2023

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst left a comment • edited Loading

Choose a reason for hiding this comment

MohitKumar020291 commented Jul 3, 2023 • edited Loading

orbeckst commented Jul 3, 2023

MohitKumar020291 commented Jul 3, 2023 • edited Loading

orbeckst left a comment

Choose a reason for hiding this comment

orbeckst commented Jul 3, 2023

MohitKumar020291 commented Jul 3, 2023

MohitKumar020291 commented Jun 29, 2023 •

edited

Loading

github-actions bot commented Jun 29, 2023 •

edited

Loading

codecov bot commented Jun 29, 2023 •

edited

Loading

orbeckst left a comment •

edited

Loading

MohitKumar020291 commented Jul 1, 2023 •

edited

Loading

MohitKumar020291 commented Jul 1, 2023 •

edited

Loading

MohitKumar020291 commented Jul 2, 2023 •

edited

Loading

MohitKumar020291 commented Jul 2, 2023 •

edited

Loading

MohitKumar020291 commented Jul 2, 2023 •

edited

Loading

orbeckst commented Jul 2, 2023 •

edited

Loading

orbeckst left a comment •

edited

Loading

MohitKumar020291 commented Jul 3, 2023 •

edited

Loading

MohitKumar020291 commented Jul 3, 2023 •

edited

Loading