updated atoms.coordinates() with atoms.positions #3467 #3576

manishsaini6421 · 2022-03-28T22:16:30Z

Fixes #

Changes made in this Pull Request:

PR Checklist

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

codecov · 2022-03-28T22:32:40Z

Codecov Report

Merging #3576 (355d619) into develop (bf95f9c) will not change coverage.
The diff coverage is 0.00%.

@@           Coverage Diff            @@
##           develop    #3576   +/-   ##
========================================
  Coverage    94.20%   94.20%           
========================================
  Files          190      190           
  Lines        24668    24668           
  Branches      3313     3313           
========================================
  Hits         23238    23238           
  Misses        1384     1384           
  Partials        46       46

Impacted Files	Coverage Δ
package/MDAnalysis/analysis/encore/covariance.py	`87.69% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf95f9c...355d619. Read the comment docs.

lilyminium · 2022-03-28T23:25:51Z

package/MDAnalysis/analysis/encore/covariance.py

@@ -215,7 +215,7 @@ def covariance_matrix(ensemble,
        # Select the same atoms in reference structure
        reference_atom_selection = reference.select_atoms(
            ensemble.get_atom_selection_string())
-        reference_coordinates = reference_atom_selection.atoms.coordinates()
+        reference_coordinates = reference_atom_selection.atoms.positions


Could you please add a test that check this fix works?

can you tell me please, how to add test?

@manishsaini6421 please write a function with code that uses this pathway. It should fail with the original .coordinates() and succeed with the new .positions. Have a look at the tests in https://github.com/MDAnalysis/mdanalysis/blob/develop/testsuite/MDAnalysisTests/analysis/test_encore.py and more generally throughout https://github.com/MDAnalysis/mdanalysis/tree/develop/testsuite/MDAnalysisTests for ideas :)

orbeckst · 2022-03-29T01:29:12Z

@manishsaini6421 please be aware that there's another PR #3479 open which would take precedence over yours. If we don't see any further progress on the other PR within 4 days we will close it as stale on 2022-04-01 and then your PR will be next in line.

See #3479 (comment) on the other PR.

Thank you for your patience.

hmacdope · 2022-03-29T03:31:43Z

@manishsaini6421 there are several other instances of .coordinates to fix, see the original issue #3467 for more info.

hmacdope

See my comment above

manishsaini6421 · 2022-03-29T06:21:49Z

@hmacdope Sir,Thanks for your support i'll do it now.

orbeckst · 2022-03-29T16:54:56Z

Hi everyone, why was this PR closed?

manishsaini6421 · 2022-03-29T16:56:32Z

why it is closed?, and my pull request is also under review till now.

EDIT: see #3577 (comment) — please continue work on this PR.

manishsaini6421 · 2022-03-29T16:59:40Z

@orbeckst Sir, please check my PR now .

orbeckst · 2022-03-29T17:09:24Z

Re-open this PR (you should see a button "Reopen and comment") and close PR #3577.

lilyminium · 2022-03-29T21:32:38Z

Hi everyone, why was this PR closed?

I think it could have been automatically closed by GitHub because @manishsaini6421 made a new PR from the same branch. There can only ever be one PR from branch A to branch B.

Edit: maybe not, since #3577 stays open...

orbeckst · 2022-03-29T22:30:48Z

@manishsaini6421 I am slightly confused why your branch manishsaini6421:shiny-new-feature only contains 1 change in this PR but contained 2 changes in the other closer PR #3577. In any case, can you please check that you are on the current branch:

git branch --show-current

should show shiny-new-feature.

Then make sure you have everything from GitHub

git pull

Then say

git status

Do you see any changes that are not committed? If so add the changes, commit, and push.

In particular, make the changes that I can see in the closed PR https://github.com/MDAnalysis/mdanalysis/pull/3577/files (in benchmarks/benchmarks/analysis/rms.py).

Check that the changes show up in this PR. If not, let us know.

manishsaini6421 · 2022-03-30T05:49:50Z

@orbeckst Sir, there is lot of confusion, please can you fix a meeting so i can do work efficiently.

IAlibay · 2022-03-30T12:40:45Z

@orbeckst Sir, there is lot of confusion, please can you fix a meeting so i can do work efficiently.

@manishsaini6421 I'll let the other @MDAnalysis/gsoc-mentors weigh in here, but we generally don't have sufficient time to have focused meetings with individual applicants during the GSoC application period. The mentors all have separate jobs outside of MDAnalysis, usually at academic institutions where teaching is currently at its peak.

I would instead suggest that you ask your questions on the mailing list or on discord, where others may get to asynchronously answer your questions.

hmacdope · 2022-03-30T22:07:26Z

@manishsaini6421 part of the issue appears to be that you are remaking, deleting or altering the shiny-new-feature branch repeatedly as it no longer contains any of the commits in this PR.

orbeckst · 2022-03-31T19:42:05Z

@manishsaini6421 as a starting point for debugging your issue, please answer my questions in #3576 (comment)

manishsaini6421 · 2022-03-31T21:11:25Z

@orbeckst Sir, this is because of creating 2nd PR.

orbeckst · 2022-04-01T17:25:31Z

This PR is now considered the primary PR for issue #3467 because PR #3479 was closed as stale. Nevertheless, the old PR contains useful information so please consult it.

orbeckst · 2022-04-01T17:26:35Z

@manishsaini6421 are you still working on this PR?

If not we will close it in 4d as stale.

orbeckst · 2022-04-01T17:33:22Z

I read #3579 (comment) and it looks as if it's not easily possible for @manishsaini6421 to continue working on this PR because their PR #3579 also uses the same branch name "shiny-new-feature". I think there's some misunderstanding for how branching with git works. In any case, I am closing this PR because it's not going to be easy for the contributor to continue.

@manishsaini6421 please open a new PR and use a distinct branch name (such as "issue-3467-replace-coordinates").

manishsaini6421 · 2022-04-01T18:21:02Z

@orbeckst ok sir i'll do it but first of all review reqired on #3579 then i can create new PR with another name.

manishsaini6421 · 2022-04-01T18:21:42Z

@orbeckst sir can you review it please.

orbeckst · 2022-04-01T22:02:19Z

We want you to do one PR after another. Focus on #3579 ; given that someone is already reviewing it, you just have to wait on the reviews there. This can take some time and we cannot drop other work as soon as there are changes on a PR; it can take a day so be patient, please.

Once PR #3579 is merged you can consider opening a new PR for issue #3467 . We are much more impressed with applicants who efficiently complete one problem than ones that open on multiple problems at the same time. Therefore, we are not going to review this closed PR.

manishsaini6421 · 2022-04-05T09:03:55Z

@orbeckst In #3579 i have made some changes after review according to changes requested but from last 2 days no one is replying on my comments . sir i am waiting for approval ,can you check it once please.

updated atoms.coordinates() with atoms.positions #3467

355d619

github-actions bot added the Component-Analysis label Mar 28, 2022

lilyminium requested changes Mar 28, 2022

View reviewed changes

orbeckst mentioned this pull request Mar 28, 2022

replace atoms.coordinates() with atoms.positions #3467

Closed

2 tasks

hmacdope requested changes Mar 29, 2022

View reviewed changes

orbeckst mentioned this pull request Mar 29, 2022

updated replace atoms.coordinates() with atoms.positions #3467 #3577

Closed

4 tasks

lilyminium reopened this Mar 29, 2022

manishsaini6421 closed this Mar 30, 2022

manishsaini6421 reopened this Mar 30, 2022

orbeckst mentioned this pull request Apr 1, 2022

Issue #3467: Replaced .coordinates() with .positions #3479

Closed

4 tasks

orbeckst linked an issue Apr 1, 2022 that may be closed by this pull request

replace atoms.coordinates() with atoms.positions #3467

Closed

2 tasks

orbeckst closed this Apr 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updated atoms.coordinates() with atoms.positions #3467 #3576

updated atoms.coordinates() with atoms.positions #3467 #3576

manishsaini6421 commented Mar 28, 2022 •

edited

Loading

codecov bot commented Mar 28, 2022 •

edited

Loading

lilyminium Mar 28, 2022

manishsaini6421 Mar 29, 2022

lilyminium Mar 29, 2022

orbeckst commented Mar 29, 2022

hmacdope commented Mar 29, 2022

hmacdope left a comment

manishsaini6421 commented Mar 29, 2022

orbeckst commented Mar 29, 2022

manishsaini6421 commented Mar 29, 2022 •

edited by orbeckst

Loading

manishsaini6421 commented Mar 29, 2022

orbeckst commented Mar 29, 2022

lilyminium commented Mar 29, 2022 •

edited

Loading

orbeckst commented Mar 29, 2022

manishsaini6421 commented Mar 30, 2022

IAlibay commented Mar 30, 2022

hmacdope commented Mar 30, 2022

orbeckst commented Mar 31, 2022

manishsaini6421 commented Mar 31, 2022

orbeckst commented Apr 1, 2022

orbeckst commented Apr 1, 2022

orbeckst commented Apr 1, 2022

manishsaini6421 commented Apr 1, 2022

manishsaini6421 commented Apr 1, 2022

orbeckst commented Apr 1, 2022 •

edited

Loading

manishsaini6421 commented Apr 5, 2022

updated atoms.coordinates() with atoms.positions #3467 #3576

updated atoms.coordinates() with atoms.positions #3467 #3576

Conversation

manishsaini6421 commented Mar 28, 2022 • edited Loading

Changes made in this Pull Request:

PR Checklist

codecov bot commented Mar 28, 2022 • edited Loading

Codecov Report

lilyminium Mar 28, 2022

Choose a reason for hiding this comment

manishsaini6421 Mar 29, 2022

Choose a reason for hiding this comment

lilyminium Mar 29, 2022

Choose a reason for hiding this comment

orbeckst commented Mar 29, 2022

hmacdope commented Mar 29, 2022

hmacdope left a comment

Choose a reason for hiding this comment

manishsaini6421 commented Mar 29, 2022

orbeckst commented Mar 29, 2022

manishsaini6421 commented Mar 29, 2022 • edited by orbeckst Loading

manishsaini6421 commented Mar 29, 2022

orbeckst commented Mar 29, 2022

lilyminium commented Mar 29, 2022 • edited Loading

orbeckst commented Mar 29, 2022

manishsaini6421 commented Mar 30, 2022

IAlibay commented Mar 30, 2022

hmacdope commented Mar 30, 2022

orbeckst commented Mar 31, 2022

manishsaini6421 commented Mar 31, 2022

orbeckst commented Apr 1, 2022

orbeckst commented Apr 1, 2022

orbeckst commented Apr 1, 2022

manishsaini6421 commented Apr 1, 2022

manishsaini6421 commented Apr 1, 2022

orbeckst commented Apr 1, 2022 • edited Loading

manishsaini6421 commented Apr 5, 2022

manishsaini6421 commented Mar 28, 2022 •

edited

Loading

codecov bot commented Mar 28, 2022 •

edited

Loading

manishsaini6421 commented Mar 29, 2022 •

edited by orbeckst

Loading

lilyminium commented Mar 29, 2022 •

edited

Loading

orbeckst commented Apr 1, 2022 •

edited

Loading