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

made universe for A and B to fix doctest fails #4023

Closed
wants to merge 6 commits into from

Conversation

Telomelonia
Copy link

@Telomelonia Telomelonia commented Feb 11, 2023

Fixes #3365

Changes made in this Pull Request:

  • a whole universe was defined to substitute A and B.
    As discussed in the issue, the expected value should not be changed.

PR Checklist

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

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on the developer mailing list so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

@codecov
Copy link

codecov bot commented Feb 11, 2023

Codecov Report

Base: 93.52% // Head: 93.52% // No change to project coverage 👍

Coverage data is based on head (8a06938) compared to base (096f799).
Patch has no changes to coverable lines.

❗ Current head 8a06938 differs from pull request most recent head 7f6b504. Consider uploading reports for the commit 7f6b504 to get more accurate results

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4023   +/-   ##
========================================
  Coverage    93.52%   93.52%           
========================================
  Files          190      190           
  Lines        25028    25028           
  Branches      3542     3542           
========================================
  Hits         23407    23407           
  Misses        1100     1100           
  Partials       521      521           
Impacted Files Coverage Δ
package/MDAnalysis/analysis/align.py 96.38% <ø> (ø)

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@hmacdope hmacdope left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the contribution!
We ask that all first time contributors introduce themselves on the developer mailing list so we can get to know you. Please also add yourself to package/AUTHORS as part of this PR and then I can merge.

@Telomelonia
Copy link
Author

@hmacdope
Sure ... will do that asap ✌️

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Please fix the indicated issues.

Run doctest or at least copy&paste a python session that shows that the code worked: if someone copies your fixed code, it should run without errors.

Also add yourself to

  • your name to AUTHORS
  • your GitHub handle @Telomelonia to top of CHANGELOG under the current release in progress
  • confirm that you have introduced yourself on the developer mailing list (mention this PR/issue)

Comment on lines 248 to 249
>>> A = mda.Universe('topol.tpr','traj.trr')
>>> B = mda.Universe('topol.tpr','traj.trr')
Copy link
Member

Choose a reason for hiding this comment

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

I suggest using A.copy()

Copy link
Member

Choose a reason for hiding this comment

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

Make B as a copy of A.

package/MDAnalysis/analysis/align.py Show resolved Hide resolved
@@ -245,7 +245,8 @@ def rotation_matrix(a, b, weights=None):
`R` can be used as an argument for
:meth:`MDAnalysis.core.groups.AtomGroup.rotate` to generate a rotated
selection, e.g. ::

>>> A = mda.Universe('topol.tpr','traj.trr')
>>> B = mda.Universe('topol.tpr','traj.trr')
>>> R = rotation_matrix(A.select_atoms('backbone').positions,
>>> B.select_atoms('backbone').positions)[0]
Copy link
Member

Choose a reason for hiding this comment

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

use ... instead of >>> to indicate line continuation

@orbeckst
Copy link
Member

Hello @Telomelonia , are you still working on the PR? If so, please address my comments. Otherwise we will close the PR as stale.

@orbeckst orbeckst added the close? Evaluate if issue/PR is stale and can be closed. label Feb 21, 2023
@Telomelonia
Copy link
Author

Hello @Telomelonia , are you still working on the PR? If so, please address my comments. Otherwise we will close the PR as stale.

@orbeckst
Sorry, I got engage into some work will see the changes

Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Please see comments and resolve the conflicts. Thanks!

package/MDAnalysis/analysis/align.py Show resolved Hide resolved
Comment on lines 248 to 249
>>> A = mda.Universe('topol.tpr','traj.trr')
>>> B = mda.Universe('topol.tpr','traj.trr')
Copy link
Member

Choose a reason for hiding this comment

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

Make B as a copy of A.

@orbeckst orbeckst removed the close? Evaluate if issue/PR is stale and can be closed. label Feb 21, 2023
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

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

Please try out your code. Copy and paste the output from a Python session that demonstrates that your code is working.

package/CHANGELOG Outdated Show resolved Hide resolved
@@ -245,9 +245,11 @@ def rotation_matrix(a, b, weights=None):
`R` can be used as an argument for
:meth:`MDAnalysis.core.groups.AtomGroup.rotate` to generate a rotated
selection, e.g. ::

>>> from MDAnalysisTests.datafiles import TPR, TRR
>>> A = mda.Universe('topol.tpr','traj.trr')
Copy link
Member

Choose a reason for hiding this comment

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

fix

Copy link
Author

Choose a reason for hiding this comment

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

Yeah. I am fixing this, but I am getting errors, and I don't know why, but I need time to understand before debugging

Copy link
Member

Choose a reason for hiding this comment

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

Show your code, then I can help.

Copy link
Author

Choose a reason for hiding this comment

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

@orbeckst, thanks, but I am currently having my semester exams 😄. So, I will try to continue after that ✌️

@orbeckst
Copy link
Member

Please also resolve conflicts.

@orbeckst orbeckst self-assigned this Feb 24, 2023
@orbeckst
Copy link
Member

@Telomelonia , we just fixed #4044, which is important for your PR because it ensures that MDAnalysisTests is available for doctests. Please Resolve conflicts and while doing so, merge the current develop branch changes into your PR.

Then make sure that all the checks pass.

@Telomelonia
Copy link
Author

Yea .. sure I will rebase it to develop branch

@orbeckst
Copy link
Member

I think you can just use the "Resolve conflicts" button in the GH userinterface (but rebase also works).

@orbeckst orbeckst added the close? Evaluate if issue/PR is stale and can be closed. label Mar 6, 2023
@orbeckst
Copy link
Member

orbeckst commented Mar 9, 2023

@Telomelonia I hope your exams went well. Can you provide a date by when you'll be able to pick up the PR again?

@Telomelonia
Copy link
Author

@Telomelonia I hope your exams went well. Can you provide a date by when you'll be able to pick up the PR again?

sure I will try to clear this PR by this weekend.

@Telomelonia
Copy link
Author

@orbeckst
I am getting errors in this line of code ... even I had imported TPR and TRR
A = mda.Universe('topol.tpr','traj.trr')
FileNotFoundError: [Errno 2] No such file or directory: 'topol.tpr'
Is there any other test I could take a and b, or I can make my own with an arbitrary angle?

@orbeckst
Copy link
Member

Sorry for the delayed response.

You need to import actual test files

from MDAnalysisTests.datafiles import TPR, TRR

and then use those.

@orbeckst orbeckst removed the close? Evaluate if issue/PR is stale and can be closed. label Mar 23, 2023
@Telomelonia
Copy link
Author

@HunterZ17
Due to some work, I won't be able to continue this PR

@Telomelonia Telomelonia deleted the doctest_fix branch March 31, 2023 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failed doc examples in align.py
3 participants