Fixing some doctests. #4118

AHMED-salah00 · 2023-04-07T20:45:06Z

Portion of #3925

Changes made in this Pull Request:

Fixed the doctests in the following files:

core/universe.py.
lib/transformations.py.
lib/util.py.

PR Checklist

Docs?
Issue raised/referenced?

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

github-actions · 2023-04-07T20:48:09Z

Linter Bot Results:

Hi @AHMED-salah00! 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/4664925964/jobs/8257693131

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

codecov · 2023-04-07T21:01:12Z

Codecov Report

Patch and project coverage have no change.

Comparison is base (fd978d2) 93.59% compared to head (08fc182) 93.59%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4118   +/-   ##
========================================
  Coverage    93.59%   93.59%           
========================================
  Files          192      192           
  Lines        25134    25134           
  Branches      4056     4056           
========================================
  Hits         23524    23524           
  Misses        1092     1092           
  Partials       518      518

Impacted Files	Coverage Δ
package/MDAnalysis/core/universe.py	`97.26% <ø> (ø)`
package/MDAnalysis/lib/transformations.py	`78.49% <ø> (ø)`
package/MDAnalysis/lib/util.py	`89.65% <ø> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

RMeli · 2023-04-10T09:23:46Z

package/MDAnalysis/lib/transformations.py

@@ -109,6 +109,7 @@
 Examples
 --------

+>>> from MDAnalysis.lib.transformations import identity_matrix


What about the other functions such as rotation_matrix, euler_from_matrix, ...? (Same comment applies elsewhere).

Hmm, I really didn't take that in mind. I will import them too.

@AHMED-salah00 I took me a while to figure this out, but #3997 added the doctests to CI. These never fail, so you need to dig them up yourself and see.

For this PR, you can go under the "Checks" tab, look for the "build_docs" test under the "GH Actions CI" menu on the left, and then look at (expand) the "doctest" phase. If you search for lib/transformations.py you can see the current failures (due to missing rotation_matrix, etc.). You can do the same for the other files you touched in this PR and hopefully fix all the current issues.

@MDAnalysis/coredevs I think here it would be more useful to have CI tests that can be incrementally turned on (instead of a single CI test for everything that never fails) when a file is addressed and fixed, so that we can ensure a PR is OK (by turning on the checks for the current files) and more importantly that no more issues are re-introduced later on. Would this be possible with doctests?

Not easily afaik - at least not with the current way we're invoking things (coming up with more complex solutions might take a while). I think the original suggestion was for reviewers to compare against the last merge commit and see if the numbers went up or down.

Thanks @RMeli for this found lots of failing docs here :)

Indeed. =) Please focus only on one or a few files at a time, to keep PRs of reasonable size (thus easier to review and merge). For the current PR I'd fix all the issues in the files you already touched:

core/universe.py

lib/transformations.py

lib/util.py

RMeli · 2023-04-10T12:37:23Z

package/MDAnalysis/lib/transformations.py

@@ -109,6 +109,7 @@
 Examples
 --------

+>>> from MDAnalysis.lib.transformations import identity_matrix


@AHMED-salah00 I took me a while to figure this out, but #3997 added the doctests to CI. These never fail, so you need to dig them up yourself and see.

For this PR, you can go under the "Checks" tab, look for the "build_docs" test under the "GH Actions CI" menu on the left, and then look at (expand) the "doctest" phase. If you search for lib/transformations.py you can see the current failures (due to missing rotation_matrix, etc.). You can do the same for the other files you touched in this PR and hopefully fix all the current issues.

@MDAnalysis/coredevs I think here it would be more useful to have CI tests that can be incrementally turned on (instead of a single CI test for everything that never fails) when a file is addressed and fixed, so that we can ensure a PR is OK (by turning on the checks for the current files) and more importantly that no more issues are re-introduced later on. Would this be possible with doctests?

package/MDAnalysis/lib/transformations.py

RMeli

lib/transformations.py LGTM:

Document: documentation_pages/lib/transformations
-------------------------------------------------
1 items passed all tests:
 160 tests in default
160 tests in 1 items.
160 passed and 0 failed.
Test passed.

For the other two files, there are still several test failing. I commented on the ones close to or related to the code you changed, but there are others as you can see in the log (especially in lib.util.py). Please double check and fix them.

RMeli · 2023-04-10T21:23:59Z

package/MDAnalysis/core/universe.py

@@ -1416,9 +1419,9 @@ def from_smiles(cls, smiles, sanitize=True, addHs=True,
        To use a different conformer generation algorithm, like ETKDGv3:

        >>> u = mda.Universe.from_smiles('CCO', rdkit_kwargs=dict(
-                                         params=AllChem.ETKDGv3()))
+            ... params=AllChem.ETKDGv3()))


/home/runner/work/mdanalysis/mdanalysis/package/doc/sphinx/source/../../../MDAnalysis/core/universe.py.rst: WARNING: ignoring invalid doctest code: ">>> u = mda.Universe.from_smiles('CCO', rdkit_kwargs=dict(\n ... params=AllChem.ETKDGv3()))\n>>> u.trajectory\n<RDKitReader with 10 frames of 9 atoms>"

Please format properly, so that this line is not invalid.

RMeli · 2023-04-10T21:27:29Z

package/MDAnalysis/core/universe.py

        >>> u.add_TopologyAttr('bfactors')
        >>> u.atoms.bfactors


This is unrelated to your changes, therefore feel free to ignore the following comment if you don't want to address it in this PR or at all.

I can see from the log that there are some warnings because bfactors has been deprecated (and will be removed in 3.0).

/home/runner/work/mdanalysis/mdanalysis/package/MDAnalysis/core/universe.py:828: DeprecationWarning: The bfactor topology attribute is only provided as an alias to the tempfactor attribute. It will be removed in 3.0. Please use the tempfactor attribute instead.

If you want/have time you could fix this warning as well (which will become an error down the line) by replacing bfactor with tempfactor (and bfactors with tempfactors).

package/MDAnalysis/lib/util.py

RMeli · 2023-04-10T21:35:44Z

package/MDAnalysis/lib/transformations.py

@@ -109,6 +109,8 @@
 Examples
 --------

+>>> from MDAnalysis.lib.transformations import *
+>>> import numpy


I think if would be better to change the whole test to be consistent with the others and use import numpy as np.

RMeli

Document: documentation_pages/core/universe
-------------------------------------------
1 items passed all tests:
  24 tests in default
24 tests in 1 items.
24 passed and 0 failed.
Test passed.

Document: documentation_pages/lib/transformations
-------------------------------------------------
1 items passed all tests:
 160 tests in default
160 tests in 1 items.
160 passed and 0 failed.
Test passed.

Document: documentation_pages/lib/util
--------------------------------------
1 items passed all tests:
  30 tests in default
30 tests in 1 items.
30 passed and 0 failed.
Test passed.

LGTM, thanks @AHMED-salah00

IAlibay

lgtm!

AHMED-salah00 added 3 commits April 7, 2023 22:23

doctests resolved issues for transformations and u.add_Residue

2cca970

Update transformations.py

78ada17

Update universe.py

3ea31aa

github-actions bot added Component-Core Component-lib labels Apr 7, 2023

AHMED-salah00 added 2 commits April 8, 2023 00:08

Update universe.py

9c32893

added missing import for greedy_splitext

5fc82dd

RMeli reviewed Apr 10, 2023

View reviewed changes

RMeli requested changes Apr 10, 2023

View reviewed changes

Fixed all doctests in transformations.py file

75f407a

IAlibay requested changes Apr 10, 2023

View reviewed changes

package/MDAnalysis/lib/transformations.py Outdated Show resolved Hide resolved

AHMED-salah00 added 2 commits April 10, 2023 18:06

import numpy as np && add more missing imports

139cacc

Solved failing doctests in core/universe.py & lib/util.py

d979976

AHMED-salah00 requested a review from RMeli April 10, 2023 19:25

RMeli requested changes Apr 10, 2023

View reviewed changes

RMeli self-assigned this Apr 10, 2023

Updates.

44647a8

AHMED-salah00 force-pushed the doctests branch from 19104ab to 44647a8 Compare April 11, 2023 07:17

Update util.py

08fc182

RMeli approved these changes Apr 11, 2023

View reviewed changes

RMeli requested a review from IAlibay April 11, 2023 09:34

IAlibay approved these changes Apr 11, 2023

View reviewed changes

IAlibay merged commit 210e05c into MDAnalysis:develop Apr 11, 2023

AHMED-salah00 deleted the doctests branch April 11, 2023 15:50

IAlibay added the Component-Docs label Sep 21, 2023

github-staff deleted a comment from AHMED-salah00 Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing some doctests. #4118

Fixing some doctests. #4118

AHMED-salah00 commented Apr 7, 2023 •

edited

Loading

github-actions bot commented Apr 7, 2023 •

edited

Loading

codecov bot commented Apr 7, 2023 •

edited

Loading

RMeli Apr 10, 2023

AHMED-salah00 Apr 10, 2023

RMeli Apr 10, 2023 •

edited

Loading

IAlibay Apr 10, 2023

AHMED-salah00 Apr 10, 2023

RMeli Apr 10, 2023

RMeli Apr 10, 2023 •

edited

Loading

RMeli left a comment

RMeli Apr 10, 2023

RMeli Apr 10, 2023

RMeli Apr 10, 2023

RMeli left a comment

IAlibay left a comment

Fixing some doctests. #4118

Fixing some doctests. #4118

Conversation

AHMED-salah00 commented Apr 7, 2023 • edited Loading

PR Checklist

github-actions bot commented Apr 7, 2023 • edited Loading

Linter Bot Results:

codecov bot commented Apr 7, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RMeli Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RMeli Apr 10, 2023 • edited Loading

Choose a reason for hiding this comment

RMeli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RMeli left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

AHMED-salah00 commented Apr 7, 2023 •

edited

Loading

github-actions bot commented Apr 7, 2023 •

edited

Loading

codecov bot commented Apr 7, 2023 •

edited

Loading

RMeli Apr 10, 2023 •

edited

Loading

RMeli Apr 10, 2023 •

edited

Loading