issue #2469 #2614

siddharthjain1611 · 2020-03-11T12:55:44Z

Fixes #2469

Changes made in this Pull Request:

PR Checklist

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

orbeckst

How do your changes address the points raised in #2469 ?

There's @lilyminium 's #2469 (comment) and talk about doc changes. Can you explain a bit more your approach?

siddharthjain1611 · 2020-03-12T02:28:14Z

How do your changes address the points raised in #2469 ?

There's @lilyminium 's #2469 (comment) and talk about doc changes. Can you explain a bit more your approach?

I tried to avoid the missing attribute error using a warning. I really couldn't understand why there's a need straight away put match_atoms=True. Please help me understand this.

orbeckst · 2020-03-12T19:10:50Z

@lilyminium could you provide a quick comment if you have time?

@PicoCentauri do you have time to look after this PR as the main reviewer?

lilyminium

Thanks for looking at this @siddharthjain1611! match_atoms is currently set to True by default because that matches the historical behaviour of the class before match_atoms was added as a keyword. It checks that the alignment selection atoms have similar element-wise masses, to make sure that you're not accidentally doing something weird, like aligning your molecule backwards.

package/MDAnalysis/analysis/align.py

PicoCentauri · 2020-03-13T10:16:11Z

@orbeckst Yes I have time to do the main review.

PicoCentauri

I agree with @lilyminium. It is probably better to add if not hasattr(ag1, 'masses') and hasattr(ag2, 'masses') directly after if math_atoms to check wether both groups contain masses. If not you can warn the user saying that "Atoms could not be matched since they don't contain masses."

PicoCentauri

Looks good!
Now please also add a test to the TestAlign class. You can use a with statement together with pytest to test your warning with pytest.warns():.

siddharthjain1611 · 2020-03-15T12:39:10Z

Looks good!
Now please also add a test to the TestAlign class. You can use a with statement together with pytest to test your warning with pytest.warns():.

Okay sir, I'll do that and get back to you as soon as possible.

siddharthjain1611 · 2020-03-15T15:37:30Z

Looks good!
Now please also add a test to the TestAlign class. You can use a with statement together with pytest to test your warning with pytest.warns():.

Sir, why are these tests being added to Test Align class and not Test Matching Atoms class.

Edit: Which new function should be created or referenced. What should be the contents for the same?

package/MDAnalysis/analysis/align.py

PicoCentauri · 2020-03-16T10:49:37Z

You are right it should be in TestGetMatchingAtoms 🙂.

Create a function like test_no_atom_masses similar to the test_match method where you create a universe without masses. Something like

reference = make_Universe()
align.get_matching_atoms(reference.atoms, reference.atoms)

should do the job.

package/MDAnalysis/analysis/align.py

testsuite/MDAnalysisTests/analysis/test_align.py

package/MDAnalysis/analysis/align.py

siddharthjain1611 · 2020-03-19T09:55:12Z

@PicoCentauri Sir, Please find some time to review my PR again.

PicoCentauri · 2020-03-19T10:24:54Z

Hey @siddharthjain1611 sorry for the delay! I will review your PR later today. For now try to check why all tests are failing.

siddharthjain1611 · 2020-03-19T11:09:28Z

Hey @siddharthjain1611 sorry for the delay! I will review your PR later today. For now try to check why all tests are failing.

Sure, Sir

PicoCentauri

Looks quite good now. Just a few things:

Why do you need the testsuite/MDAnalysisTests/data/rmsfit_adk_dims.dcd? It is not used in any of your tests.
Due to your fix the test_fit_translation_no_masses is failing since fit_translation now raises an ValueError in line 104 rather an AttributeError in line 102. For now I would just change AttributeError by ValueError in the test.
In addition add yourself the the AUTHORS and add your changes to the CHANGELOG.

siddharthjain1611 · 2020-03-19T15:53:23Z

Looks quite good now. Just a few things:

Why do you need the testsuite/MDAnalysisTests/data/rmsfit_adk_dims.dcd? It is not used in any of your tests.

Due to your fix the test_fit_translation_no_masses is failing since fit_translation now raises an ValueError in line 104 rather an AttributeError in line 102. For now I would just change AttributeError by ValueError in the test.

In addition add yourself the the AUTHORS and add your changes to the CHANGELOG.

I'll delete the file testsuite/MDAnalysisTests/data/rmsfit_adk_dims.dcd. And I'll add myself to authors and will also edit the changelog. Also, wondering sir why does creating a universe without masses raises an error? Shouldn't it be allowed?

My review was a blocking comment until the PR was more substantial. Other revieers have taken over.

siddharthjain1611 · 2020-03-20T05:32:59Z

@orbeckst Sir, is the pr not appropriate? Does it need more work?

PicoCentauri · 2020-03-20T08:10:15Z

Also, wondering sir why does creating a universe without masses raises an error? Shouldn't it be allowed?

Your are right. The creation is valid but the method fit_translation() with the option weights="mass" requires an universe witth masses and therefore should raise an error if they don't exist.

siddharthjain1611 · 2020-03-20T08:36:09Z

Is this pr ready for merging?

PicoCentauri · 2020-03-20T09:12:32Z

First you have to fix the test_fit_translation_no_masses test. If the tests are not passing we can not merge.

siddharthjain1611 · 2020-03-20T09:41:51Z

First you have to fix the test_fit_translation_no_masses test. If the tests are not passing we can not merge.

I'll fix that as soon as possible.

siddharthjain1611 · 2020-03-20T12:18:44Z

@PicoCentauri Sir, For that problem, I feel setting the mass to 1 solve the problem.
Is below code correct:
with pytest.raises(ValueError):
fit_translation(test_u, ref_u, weights=1)(ts)

PicoCentauri · 2020-03-20T12:44:57Z

You really want that the test raises an error here. The purpose of tests is not only to check wether code works but also to check that it raises errors under certain conditions.

siddharthjain1611 · 2020-03-20T13:36:29Z

You really want that the test raises an error here. The purpose of tests is not only to check wether code works but also to check that it raises errors under certain conditions.

Okay sir, would just a warning be sufficient?

PicoCentauri

I checked our changes locally and if you do these changes it should work out

PicoCentauri · 2020-03-26T16:33:26Z

package/MDAnalysis/analysis/align.py

-                errmsg = ("Failed to automatically find matching atoms: created empty selections. " +
-                          "Try to improve your selections for mobile and reference.")
+                errmsg = ("Failed to automatically find matching atoms: created empty selections. "
+                            "Try to improve your selections for mobile and reference.")


This is still to be fixed

package/MDAnalysis/analysis/align.py

package/MDAnalysis/transformations/fit.py

siddharthjain1611 · 2020-03-27T03:22:33Z

@PicoCentauri @lilyminium Please review this.

PicoCentauri

Very good. Just remove the spaces and I am happy

package/MDAnalysis/analysis/align.py

siddharthjain1611 · 2020-03-27T12:46:51Z

@PicoCentauri Sir, I think the spaces look fine now, Please find some time to have a look :)

PicoCentauri · 2020-03-27T14:41:35Z

package/MDAnalysis/analysis/align.py

-            logger.error(errmsg)
-            raise SelectionError(errmsg)
+        if (not hasattr(ag1, 'masses') or not hasattr(ag2, 'masses')):
+            # # WARNING:


Finally, remove this superfluous comment if you want.

I saw a comment of this kind somewhere else, therefore I wrote it.

Having the comment there is not bad but I would say not needed because the code itself clearly says "warning".

However, I see a PEP8 issue below...

package/MDAnalysis/analysis/align.py

lilyminium

Looking good, @siddharthjain1611, nearly there. Please remove rmsfit_adk_dims.dcd also. Thank you!

package/MDAnalysis/analysis/rms.py

testsuite/MDAnalysisTests/analysis/test_align.py

testsuite/MDAnalysisTests/analysis/test_rms.py

orbeckst · 2020-03-27T17:14:01Z

@PicoCentauri – you have the power to dismiss @lilyminium 's request if you think that her points have been addressed and if she doesn't have the time to look at the PR. If you do this, you just write a short justification message and go ahead.

orbeckst · 2020-03-27T17:14:30Z

Oops – apparently @lilyminium commented 30 seconds ago.... ignore my previous comment ;-).

Co-Authored-By: Oliver Beckstein <orbeckst@gmail.com>

lilyminium · 2020-03-27T18:57:35Z

Thanks @siddharthjain1611, I'm happy with this but let's wait for Travis to go green. Don't forget to remove rmsfit_adk_dims.dcd :-)

siddharthjain1611 · 2020-03-27T19:21:26Z

Thanks @siddharthjain1611, I'm happy with this but let's wait for Travis to go green. Don't forget to remove rmsfit_adk_dims.dcd :-)

Sorry, ma'am wasn't able to find that file in pr. I think I already discarded that in my system, but even if by mistake I pushed it. Please do point it out. I'll happily do it.

lilyminium · 2020-03-27T21:00:12Z

@siddharthjain1611 You can delete this file on GitHub itself:

https://github.com/MDAnalysis/mdanalysis/pull/2614/files#diff-216eba108275dddf90ef894a79bb8699

siddharthjain1611 · 2020-03-28T02:23:57Z

@siddharthjain1611 You can delete this file on GitHub itself:

https://github.com/MDAnalysis/mdanalysis/pull/2614/files#diff-216eba108275dddf90ef894a79bb8699

Done ma'am.

lilyminium

Awesome, looks good to me and thanks for the contribution! I'll let @PicoCentauri do the honours.

siddharthjain1611 · 2020-03-28T05:07:47Z

Awesome, looks good to me and thanks for the contribution! I'll let @PicoCentauri do the honours.

Thank you, ma'am. Got to learn a lot from both of you.
@PicoCentauri

siddharthjain1611 · 2020-03-28T12:39:34Z

@PicoCentauri @lilyminium is the PR ready for merging?

lilyminium · 2020-03-28T21:47:04Z

@PicoCentauri seems to be busy so I've just merged it now. Congratulations on your first contribution!

@tbengtsen

Update of AUTHORS and CHANGELOG with inferred author contributions. * Removed duplicate mattwthompson in 0.20.0 changelog entry.: mattwthompson was placed twice by accident, this removes this duplication. * Addition of missing authors. An retrospective analysis of the authors found via `git shortlog -s -n --email --branches="develop"` found several commits by authors which were not present in the `AUTHORS.md` file. - Zhenbo Li: commited via PR: Started tests for gnm. #803 and Make Travis run tests on OSX. #771, - Jenna M. Swarthout via PR Update CoC according to suggestions from current CoC committee #4289 and Point to new reporting form link (owned by conduct@mdanalysis.org) #4298, - Bradley Dice via PR Fix GSD Windows compatibility #2384 , - David Minh via PR #2668 There seemed to be no indications in the above mentioned PRs that the author did not want to be in the authors file, it looks like it was just forgotten. * Addition of missing entries from the changelog Continued cross referencing of the git shortlog output but also accounting for the changelog identified several individuals that had not been included in the changelog entries for the release they contributed under. They were added to the relevant entry of the changelog based on the merge commit date. Please note that for Tone Bengsten, we a) had no github handle (so they were assigned @tbengtsen), and b) no specific commit. Given we know that this individual was including alongside the encore merge, they were assigned to the 0.16.0 release. * Update CHANGELOG * PR #1218 * PR #1284 and #1408 * PR #4109 * based on git history * PRs #803 and #771 (author addition, changelog addition) * PR #2255 and #2221 * PR #1225 * PR #4289 and #4298 * PR #4031 * PR #4085 * PR #3635 * PR #2356 * PR #2559 * No GH handle - Encore author addition @tbengtsen * PR #4184 * PR #2614 * PR #2219 * PR #2384 * PR #2668 * Add missing entry for Jenna Thanks to @fiona-naughton for helping out with digging into this data :) Co-authored-by: Fiona Naughton <fiona@mdanalysis.org> Co-authored-by: Oliver Beckstein <orbeckst@mdanalysis.org>

siddharthjain1611 mentioned this pull request Mar 11, 2020

issue #2469 #2586

Closed

4 tasks

orbeckst added the GSOC Starter label Mar 11, 2020

orbeckst previously requested changes Mar 12, 2020

View reviewed changes

lilyminium requested changes Mar 13, 2020

View reviewed changes

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

PicoCentauri requested changes Mar 13, 2020

View reviewed changes

PicoCentauri requested changes Mar 15, 2020

View reviewed changes

PicoCentauri requested changes Mar 16, 2020

View reviewed changes

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

PicoCentauri requested changes Mar 17, 2020

View reviewed changes

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

testsuite/MDAnalysisTests/analysis/test_align.py Outdated Show resolved Hide resolved

lilyminium reviewed Mar 17, 2020

View reviewed changes

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

PicoCentauri requested changes Mar 19, 2020

View reviewed changes

orbeckst assigned PicoCentauri Mar 20, 2020

PicoCentauri requested changes Mar 26, 2020

View reviewed changes

update fit.py

88e361b

PicoCentauri requested changes Mar 27, 2020

View reviewed changes

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

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

pep8

4435b96

PicoCentauri reviewed Mar 27, 2020

View reviewed changes

PicoCentauri approved these changes Mar 27, 2020

View reviewed changes

orbeckst reviewed Mar 27, 2020

View reviewed changes

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

orbeckst reviewed Mar 27, 2020

View reviewed changes

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

lilyminium requested changes Mar 27, 2020

View reviewed changes

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

testsuite/MDAnalysisTests/analysis/test_align.py Outdated Show resolved Hide resolved

testsuite/MDAnalysisTests/analysis/test_rms.py Outdated Show resolved Hide resolved

siddharthjain1611 and others added 3 commits March 27, 2020 23:01

Update package/MDAnalysis/analysis/align.py

31f10b8

Co-Authored-By: Oliver Beckstein <orbeckst@gmail.com>

Update package/MDAnalysis/analysis/align.py

040b33f

Co-Authored-By: Oliver Beckstein <orbeckst@gmail.com>

PEP and Value Error

0decdf7

removed file

96a8f1f

lilyminium approved these changes Mar 28, 2020

View reviewed changes

lilyminium merged commit 60eb9d8 into MDAnalysis:develop Mar 28, 2020

fiona-naughton added Component-Docs Component-Analysis labels Sep 26, 2023

IAlibay added a commit that referenced this pull request Dec 18, 2023

PR #2614

d4a216d

issue #2469 #2614

issue #2469 #2614

Conversation

siddharthjain1611 commented Mar 11, 2020 • edited Loading

Changes made in this Pull Request:

PR Checklist

orbeckst left a comment

Choose a reason for hiding this comment

siddharthjain1611 commented Mar 12, 2020

orbeckst commented Mar 12, 2020

lilyminium left a comment • edited Loading

Choose a reason for hiding this comment

PicoCentauri commented Mar 13, 2020

PicoCentauri left a comment

Choose a reason for hiding this comment

PicoCentauri left a comment • edited Loading

Choose a reason for hiding this comment

siddharthjain1611 commented Mar 15, 2020

siddharthjain1611 commented Mar 15, 2020 • edited Loading

PicoCentauri commented Mar 16, 2020

siddharthjain1611 commented Mar 19, 2020

PicoCentauri commented Mar 19, 2020

siddharthjain1611 commented Mar 19, 2020

PicoCentauri left a comment • edited Loading

Choose a reason for hiding this comment

siddharthjain1611 commented Mar 19, 2020 • edited Loading

siddharthjain1611 commented Mar 20, 2020

PicoCentauri commented Mar 20, 2020

siddharthjain1611 commented Mar 20, 2020

PicoCentauri commented Mar 20, 2020

siddharthjain1611 commented Mar 20, 2020

siddharthjain1611 commented Mar 20, 2020 • edited Loading

PicoCentauri commented Mar 20, 2020

siddharthjain1611 commented Mar 20, 2020 • edited Loading

PicoCentauri left a comment

Choose a reason for hiding this comment

PicoCentauri Mar 26, 2020

Choose a reason for hiding this comment

siddharthjain1611 commented Mar 27, 2020

PicoCentauri left a comment

Choose a reason for hiding this comment

siddharthjain1611 commented Mar 27, 2020

PicoCentauri Mar 27, 2020 • edited Loading

Choose a reason for hiding this comment

siddharthjain1611 Mar 27, 2020

Choose a reason for hiding this comment

orbeckst Mar 27, 2020

Choose a reason for hiding this comment

lilyminium left a comment

Choose a reason for hiding this comment

orbeckst commented Mar 27, 2020

orbeckst commented Mar 27, 2020

lilyminium commented Mar 27, 2020

siddharthjain1611 commented Mar 27, 2020

lilyminium commented Mar 27, 2020

siddharthjain1611 commented Mar 28, 2020

lilyminium left a comment

Choose a reason for hiding this comment

siddharthjain1611 commented Mar 28, 2020 • edited Loading

siddharthjain1611 commented Mar 28, 2020 • edited Loading

lilyminium commented Mar 28, 2020

siddharthjain1611 commented Mar 11, 2020 •

edited

Loading

lilyminium left a comment •

edited

Loading

PicoCentauri left a comment •

edited

Loading

siddharthjain1611 commented Mar 15, 2020 •

edited

Loading

PicoCentauri left a comment •

edited

Loading

siddharthjain1611 commented Mar 19, 2020 •

edited

Loading

siddharthjain1611 commented Mar 20, 2020 •

edited

Loading

siddharthjain1611 commented Mar 20, 2020 •

edited

Loading

PicoCentauri Mar 27, 2020 •

edited

Loading

siddharthjain1611 commented Mar 28, 2020 •

edited

Loading

siddharthjain1611 commented Mar 28, 2020 •

edited

Loading