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.

Sign up for GitHub

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

Jump to bottom

Update distances.py #1275

Closed

Shtkddud123 wants to merge 3 commits into MDAnalysis:develop from Shtkddud123:develop

Contributor

Shtkddud123 commented Mar 30, 2017 •

edited by orbeckst

Loading

Fixes #1262

Changes made in this Pull Request:

Added a function distance() which works for a single atomic/molecular distance which works as a compliment of the distance_array() function.

PR Checklist

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


          Update distances.py

2c317cb

Contributor

jbarnoud commented Mar 31, 2017

Thank you for working on this issue.

Your change broke the test. Among the flow of errors, only that one is relevant:

======================================================================
ERROR: Failure: IndentationError (unexpected indent (distances.py, line 218))
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/MDAnalysis/mdanalysis/miniconda/envs/pyenv/lib/python2.7/site-packages/nose/loader.py", line 418, in loadTestsFromName
    addr.filename, addr.module)
  File "/home/travis/build/MDAnalysis/mdanalysis/miniconda/envs/pyenv/lib/python2.7/site-packages/nose/importer.py", line 47, in importFromPath
    return self.importFromDir(dir_path, fqname)
  File "/home/travis/build/MDAnalysis/mdanalysis/miniconda/envs/pyenv/lib/python2.7/site-packages/nose/importer.py", line 94, in importFromDir
    mod = load_module(part_fqname, fh, filename, desc)
  File "/home/travis/build/MDAnalysis/mdanalysis/miniconda/envs/pyenv/lib/python2.7/site-packages/MDAnalysisTests/analysis/test_align.py", line 26, in <module>
    import MDAnalysis as mda
  File "/home/travis/build/MDAnalysis/mdanalysis/miniconda/envs/pyenv/lib/python2.7/site-packages/MDAnalysis/__init__.py", line 178, in <module>
    from .lib import log
  File "/home/travis/build/MDAnalysis/mdanalysis/miniconda/envs/pyenv/lib/python2.7/site-packages/MDAnalysis/lib/__init__.py", line 36, in <module>
    from . import distances  # distances relies on mdamath
  File "/home/travis/build/MDAnalysis/mdanalysis/miniconda/envs/pyenv/lib/python2.7/site-packages/MDAnalysis/lib/distances.py", line 218
    name1 = "bynum" + " " + str(atom1) # concatenate string for first selection
    ^
IndentationError: unexpected indent

The tests are run automatically by TravisCI, but you can run them locally to find issues faster. Have a look at our guide to read how.

orbeckst reviewed

View reviewed changes

Member

orbeckst left a comment

Thanks for taking on this problem. I added a few quick comments, hopefully they are helpful as you go along.

package/MDAnalysis/lib/distances.py Outdated

@@ @@ -206,6 +206,81 @@ def _check_lengths_match(*arrays): @@
                   if not all( a.shape == ref for a in arrays):
                       raise ValueError("Input arrays must all be same shape"
                                        "Got {0}".format([a.shape for a in arrays]))
+              ## NOT COMPLETE - NEED REFINEMENT
+              def distance(atom1,atom2,box=None):

Member

orbeckst Mar 31, 2017

Please follow PEP8 style, here "leave space after commas".

package/MDAnalysis/lib/distances.py Outdated

+                     TODO: refinement of the array structure to take advantage of numpy arrays, and generalize the input var
+                     """
+                      name1 = "bynum" + " " + str(atom1) # concatenate string for first selection

Member

orbeckst Mar 31, 2017

indentation error

package/MDAnalysis/lib/distances.py Outdated

@@ @@ -206,6 +206,81 @@ def _check_lengths_match(*arrays): @@
                   if not all( a.shape == ref for a in arrays):
                       raise ValueError("Input arrays must all be same shape"
                                        "Got {0}".format([a.shape for a in arrays]))
+              ## NOT COMPLETE - NEED REFINEMENT
+              def distance(atom1,atom2,box=None):

Member

orbeckst Mar 31, 2017

The call signature should be

distance(a, b, box=None)

where a and b are coordinates (numpy arrays of shape (3,)).

No functions in MDAnalysis.lib will make explicit reference to atoms or other "core data structures". Look at distance_array().

package/MDAnalysis/lib/distances.py Outdated

+                      name1 = "bynum" + " " + str(atom1) # concatenate string for first selection
+                      name2 = "bynum" + " " + str(atom2) # concatenate string for the second selection
+                      selection1 = u.select_atoms(name1) # select from universe

Member

orbeckst Mar 31, 2017

YOu don't need any selections here. Work with raw coordinates.

package/MDAnalysis/lib/distances.py Outdated

+                                  if abs(selection1_selection2_array[i][2]) > u.dimensions[5]:
+                                     # Z axis minimum image - follows the LAMMPS convention
+                                      if selection1_selection2_array[i][2] < 0.0:
+                                             selection1_selection2_array[i][2] += u.dimensions[5]

Member

orbeckst Mar 31, 2017

If you're using numpy arrays, use numpy array operations, e.g.

selection1_selection2_array[i, 0:3] += u.dimensions [3:6]

package/MDAnalysis/lib/distances.py Outdated

+                          box_type = box_check(box) ## Check type of box and store it in box_type variable
+                          if box_type == 'tri_box':
+                              for i in range(len(selection1_selection2_array)):

Member

orbeckst Mar 31, 2017

This looks all a bit complicated. However, implement your tests and then see if your solution works. Once you have one solution it can be improved. But tests are important early on.

package/MDAnalysis/lib/distances.py Outdated


		return separation

Member

orbeckst Mar 31, 2017

Implement tests!!!!

orbeckst added the Work in progress label

Member

orbeckst commented Mar 31, 2017

@Shtkddud123 , if you want to get in a PR for GSoC then I recommend you're not doing this one (this will take longer!) but something very easy such as #1246 or look at #597 (and just write a test that tests raising of an exception that is not being tested yer – the issue #597 has more details).

Contributor Author

Shtkddud123 commented Mar 31, 2017 via email

Dear Dr Beckstein, I see. I apologise for the fairly rough code - I was aware of that, but I think I rushed it to try to make it to April 3rd. I'm still invested in it, so I'll get back to it with your suggestions. For the meantime, I'll try to resolve one of the issues you suggested. Thanks Sang

On Fri, Mar 31, 2017 at 9:44 AM, Oliver Beckstein ***@***.***> wrote: @Shtkddud123 <https://github.com/Shtkddud123> , if you want to get in a PR for GSoC then I recommend you're not doing this one (this will take longer!) but something very easy such as #1246 <#1246> or look at #597 <#597> (and just write a test that tests raising of an exception that is not being tested yer – the issue #597 <#597> has more details). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEg_lKv9zNZfzcyiYAXAZ11IJRpEzEGuks5rrLzWgaJpZM4MuvMw> .

Shtkddud123 added 2 commits

April 2, 2017 19:23


          Update distances.py


          Update PDB.py

849689a

Member

orbeckst commented May 15, 2017

@Shtkddud123 are you interested in continuing this PR or can I close it for the time being?

Contributor Author

Shtkddud123 commented May 15, 2017 via email

Dear Dr Beckstein, Sorry for the lack of notice. I will be doing it in the next week or so - sorry, personal situations have got in the way, but I am still committed. Thank you. Yours Sincerely Sang

On Mon, May 15, 2017 at 9:46 PM, Oliver Beckstein ***@***.***> wrote: @Shtkddud123 <https://github.com/shtkddud123> are you interested in continuing this PR or can I close it for the time being? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEg_lJPkAKKt11fWDv1Zj4lMKj1rXVhmks5r6LmtgaJpZM4MuvMw> .

Member

orbeckst commented May 16, 2017 via email

Ok, just checking. Btw no need for formal address on GitHub ---- just @orbeckst

Am May 15, 2017 um 15:12 schrieb Sang Young Noh ***@***.***>: Dear Dr Beckstein, Sorry for the lack of notice. I will be doing it in the next week or so - sorry, personal situations have got in the way, but I am still committed. Thank you. Yours Sincerely Sang On Mon, May 15, 2017 at 9:46 PM, Oliver Beckstein ***@***.***> wrote: > @Shtkddud123 <https://github.com/shtkddud123> are you interested in > continuing this PR or can I close it for the time being? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#1275 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AEg_lJPkAKKt11fWDv1Zj4lMKj1rXVhmks5r6LmtgaJpZM4MuvMw> > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

Member

orbeckst commented Jul 10, 2017

Hi @Shtkddud123, thanks for your interest in contributing. There hasn't been much more activity recently and there isn't code in the PR yet that relates to distances and it shows "unknown repository" as the source. Therefore I am going to close this PR. Please submit a new PR if you find the time to work on this problem again. Hope to see you back here!

orbeckst closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Work in progress