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

WIP: Sklearn kdtree #395

Merged
merged 17 commits into from Aug 24, 2015

Conversation

Projects
None yet
2 participants
@kain88-de
Member

kain88-de commented Aug 16, 2015

Fixes #383

I have completely removed the old KDTree library. Functions that have used the old class
directly have been adopted to use the scikit-learn implementation. The AtomNeighborSearch class
was a nice abstraction that worked directly with AtomGroups, it is still available but internally it
uses the new KDTree implementation. I tried to make the commits self contained which should make it easier to review this.

The tests have been updated for the new AtomNeighborSearch API.

TODO:

  • write Doc strings for AtomNeighborSearch
  • add removal of NeighborSearch to changelog
  • deprecate KDTree completely with notice to use scikit-learn instead
  • fix deprecation test failure
@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Aug 17, 2015

Member

Btw what are is the purpose of the depcreation test that is failing? Should it test that depcreated imports are still available? In that case I would remove that specific test because I have removed the module. AtomNeighborSearch now lives in it's own module, where I'm thinking if I should move it to the analysis sub-module since it is only used there.

Member

kain88-de commented Aug 17, 2015

Btw what are is the purpose of the depcreation test that is failing? Should it test that depcreated imports are still available? In that case I would remove that specific test because I have removed the module. AtomNeighborSearch now lives in it's own module, where I'm thinking if I should move it to the analysis sub-module since it is only used there.

@richardjgowers

This comment has been minimized.

Show comment
Hide comment
@richardjgowers

richardjgowers Aug 19, 2015

Member

I think the failing deprecation test is that KDTree used to live in MDAnalysis.KDTree and we moved it to MDAnalysis.lib.KDTree, but we want the old address to work (for now), and issue a deprecation warning. So there's a stub module which issues the warning then imports the other module.

Member

richardjgowers commented Aug 19, 2015

I think the failing deprecation test is that KDTree used to live in MDAnalysis.KDTree and we moved it to MDAnalysis.lib.KDTree, but we want the old address to work (for now), and issue a deprecation warning. So there's a stub module which issues the warning then imports the other module.

@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Aug 19, 2015

Member

I guess then I could readd the stub module and load the new atomneighbors instead and issue a
warning about the move and rename. I think I will also move the atomneighbors module into MDAnalysis.analysis since it is only used there in MDAnalysis and I figure it is something people want to use it in their analysis.

Member

kain88-de commented Aug 19, 2015

I guess then I could readd the stub module and load the new atomneighbors instead and issue a
warning about the move and rename. I think I will also move the atomneighbors module into MDAnalysis.analysis since it is only used there in MDAnalysis and I figure it is something people want to use it in their analysis.

@richardjgowers

This comment has been minimized.

Show comment
Hide comment
@richardjgowers

richardjgowers Aug 19, 2015

Member

The submodule .analysis is complete tools which can have MDA dependencies,
whilst .lib is building blocks without MDA dependencies. I need to properly
catch up on this to figure out where this should live.

On 1:24PM, Wed, 19 Aug 2015 kain88-de notifications@github.com wrote:

I guess then I could readd the stub module and load the new atomneighbors
instead and issue a
warning about the move and rename. I think I will also move the
atomneighbors module into MDAnalysis.analysis since it is only used there
in MDAnalysis and I figure it is something people want to use it in their
analysis.


Reply to this email directly or view it on GitHub
#395 (comment)
.

Member

richardjgowers commented Aug 19, 2015

The submodule .analysis is complete tools which can have MDA dependencies,
whilst .lib is building blocks without MDA dependencies. I need to properly
catch up on this to figure out where this should live.

On 1:24PM, Wed, 19 Aug 2015 kain88-de notifications@github.com wrote:

I guess then I could readd the stub module and load the new atomneighbors
instead and issue a
warning about the move and rename. I think I will also move the
atomneighbors module into MDAnalysis.analysis since it is only used there
in MDAnalysis and I figure it is something people want to use it in their
analysis.


Reply to this email directly or view it on GitHub
#395 (comment)
.

@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Aug 19, 2015

Member

Once you found out where it should live I will finish my TODO-list to have this mergeable.

Btw it is ok that I removed the NeighborSearch class? I didn't see any particular value for a
KDTree wrapper in MDAnalysis and I figured if people want to use a KDTree on arbitrary coordinates
it might be better if they use one of the common ML-python packages

Member

kain88-de commented Aug 19, 2015

Once you found out where it should live I will finish my TODO-list to have this mergeable.

Btw it is ok that I removed the NeighborSearch class? I didn't see any particular value for a
KDTree wrapper in MDAnalysis and I figured if people want to use a KDTree on arbitrary coordinates
it might be better if they use one of the common ML-python packages

kain88-de added some commits Aug 9, 2015

Use scikit-learn KDTree in around selection
We switch from the old copied and here maintained Biopython version of
KDTree to the scikit-learn. For one this will result in a speed increase
and secondly it will allow us to remove the old python 3 incompatible
SWIG module. Plus less code to test and maintain.

For spherical selections the tests fail right now because they are not
ported yet.
Port Sphericallayer selection for new KDTree
Continue to get rid of the old KDTree module
Remove KDTree module
This removes all appearences of the KDTree Module and it's
Neighbor classes as well. I will reimplement the AtomNeighbor
class again later.
Re add AtomNeighborsearch class
This is a refactored implementation of this class based on using the
sklearn KDTree. The search_all method is currently not implemented. The
tests aren't currently passing that will be fixed in later commits.
Use new AtomNeighborSearch in hbond_analysis
PEP8-tify the functions I touched as well.
Use Bio.KDTree for selections
This way the KDTree removal doesn't require any new dependencies.
Add MDAnalysis.KDTree again
Explain that the KDTRee has been removed and that this now only
loads the NeighborSearch package.
@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Aug 23, 2015

Member

Tests are all passing now and I fixed the doc string for the atomneihgborsearch class. The I didn't test the density analysis because a unit-test for it doesn't exists.

Anything else missing?

Member

kain88-de commented Aug 23, 2015

Tests are all passing now and I fixed the doc string for the atomneihgborsearch class. The I didn't test the density analysis because a unit-test for it doesn't exists.

Anything else missing?

@@ -0,0 +1,75 @@
import numpy

This comment has been minimized.

@richardjgowers

richardjgowers Aug 24, 2015

Member

This file will need a header comment for the rst docs won't it?

@richardjgowers

richardjgowers Aug 24, 2015

Member

This file will need a header comment for the rst docs won't it?

This comment has been minimized.

@kain88-de

kain88-de Aug 24, 2015

Member

I added a header comment. I'm not very familiar with the rst docs. If I missed defining some rst specific stuff please tell me. I would also appreciate a link to the appropriate sphinx documentation for this.

@kain88-de

kain88-de Aug 24, 2015

Member

I added a header comment. I'm not very familiar with the rst docs. If I missed defining some rst specific stuff please tell me. I would also appreciate a link to the appropriate sphinx documentation for this.

@@ -20,10 +20,9 @@
================================================================
"""
__all__ = ['KDTree', 'log', 'transformations', 'util', 'mdamath', 'distances',

This comment has been minimized.

@richardjgowers

richardjgowers Aug 24, 2015

Member

Add NeighborSearch here?

@richardjgowers

richardjgowers Aug 24, 2015

Member

Add NeighborSearch here?

@richardjgowers richardjgowers self-assigned this Aug 24, 2015

@richardjgowers richardjgowers added this to the 0.11 milestone Aug 24, 2015

@richardjgowers

This comment has been minimized.

Show comment
Hide comment
@richardjgowers

richardjgowers Aug 24, 2015

Member

Ok header looks fine. What needs doing now is for the .rst stubs to be updated. If you go to doc/sphinx/source/documentation_pages/ there is a lot of .rst files that mirror the layout of the package. You need to update these (KDTree and lib) so that they reflect the new structure. I think this will be, deleting the KDTree directory, add a new .rst for NeighborSearch (copy another submodule, should just be 2 lines) and add a link to lib_modules.rst

Link to links:
https://github.com/MDAnalysis/mdanalysis/wiki/WritingDocumentation#online-docs-format-sphinx-restructuredtext

Member

richardjgowers commented Aug 24, 2015

Ok header looks fine. What needs doing now is for the .rst stubs to be updated. If you go to doc/sphinx/source/documentation_pages/ there is a lot of .rst files that mirror the layout of the package. You need to update these (KDTree and lib) so that they reflect the new structure. I think this will be, deleting the KDTree directory, add a new .rst for NeighborSearch (copy another submodule, should just be 2 lines) and add a link to lib_modules.rst

Link to links:
https://github.com/MDAnalysis/mdanalysis/wiki/WritingDocumentation#online-docs-format-sphinx-restructuredtext

Update sphinx documentation that KDTree is removed
I also added a page for the new NeighborSearch module
@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Aug 24, 2015

Member

I removed the old KDTree rst files and added a short one for the new NeighborSearch module. The formatting is different then from the other include in lib_modules.rst. But I have no idea why.

Plus I suddenly have all these import error where I have no real idea what might have caused them.

This is an example for a failed import.
https://travis-ci.org/MDAnalysis/mdanalysis/builds/77053307#L459

I assume loading NeighborSearch in lib/__init__.py somehow has caused circular imports with AtomGroup. Removing that import fixes the import-errors I'm seeing. @richardjgowers
Do you have any idea how to fix this properly? Currently I can only think of using

from ..exceptions import SelectionError, .....

in AtomgGourp.py

Member

kain88-de commented Aug 24, 2015

I removed the old KDTree rst files and added a short one for the new NeighborSearch module. The formatting is different then from the other include in lib_modules.rst. But I have no idea why.

Plus I suddenly have all these import error where I have no real idea what might have caused them.

This is an example for a failed import.
https://travis-ci.org/MDAnalysis/mdanalysis/builds/77053307#L459

I assume loading NeighborSearch in lib/__init__.py somehow has caused circular imports with AtomGroup. Removing that import fixes the import-errors I'm seeing. @richardjgowers
Do you have any idea how to fix this properly? Currently I can only think of using

from ..exceptions import SelectionError, .....

in AtomgGourp.py

Fix circular import.
The problem here is that an import statement actually EXECUTES all the
code that is defined inside of the modules __init__.py. The problem now
is that the lib module was loaded before the execptions were loaded,
note that lib/__init__.py now loads `NeighborSearch.py` which in return
loads `AtomGroup.py` which loads `SelectionError` which is defined later
int the top `__init__.py`. This means we have a circular import and
everything just stops working. By importing the exeptions first
everything will work fine.
@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Aug 24, 2015

Member

Plus I suddenly have all these import error where I have no real idea what might have caused them

Yup it was a circular import see 80078fe.

Member

kain88-de commented Aug 24, 2015

Plus I suddenly have all these import error where I have no real idea what might have caused them

Yup it was a circular import see 80078fe.

@richardjgowers

This comment has been minimized.

Show comment
Hide comment
@richardjgowers

richardjgowers Aug 24, 2015

Member

Awesome

Member

richardjgowers commented Aug 24, 2015

Awesome

richardjgowers added a commit that referenced this pull request Aug 24, 2015

@richardjgowers richardjgowers merged commit 248b489 into MDAnalysis:develop Aug 24, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kain88-de kain88-de deleted the kain88-de:sklearn-kdtree branch Sep 12, 2015

orbeckst added a commit that referenced this pull request Sep 15, 2016

updated AUTHORS
- changed "Maintained by Oliver Beckstein and Elizabeth Denning" to
  "Maintained by the MDAnalysis Core Devs Team"
- added Wouter Boomsma (for PR #976)
- removed explicit mentions for KDTree (Hamelryck) because since 0.11.0
  we do no longer include the actual code (see PR #395 and #383)
- removed explicit mentions for helanal (Hall) because Ben Hall is
  a contributing author

abiedermann pushed a commit to abiedermann/mdanalysis that referenced this pull request Jan 5, 2017

iQIcBAABCAAGBQJX2vOdAAoJEObgNkBNX4V06+IP/AqlOu61RPnDLG6EYc+xGllQ
 JmhX7IYP+2pnu4iv7mzp+xy8W5x1pzFOZhzfS4LslB2Ac+4SENM1eAZebiZQYSYl
 qyYkj9bLS2RqC40dQVfAA4dMrAuWpNfgZkTeCCU7pm29TodN0hmljNG/rLoGG/iH
 ieA+pSultKwyryY0UZXHAMmNRIViEfy1VsjE2Hswc/AIiY9W1yfLONXVjtELjFsU
 jv4u1ljR+lhuP6ZllEwucKeD5ojQDyXBU9WXPi2bUwFEFpNdn5zXVRTV/kntb86d
 sbOE1TQpy+WeSNTyqmEtdjQRuLH21Jm9kTnWFjkKjw2lL+odUOAoOEC4tLSin30i
 6KK2lVvSfXJi9e5XOL+nXWqvwr+jU5jqNV+PC60aEe9N1xhVapYroY84bIJwcuYQ
 2Axu0vJWNLOpnLGAH7kYYaeEFJdsZt283kDy1paagWmXqyy3p3hh1VD8NEWWcXQ4
 Da07VzZh61RvLHFWIzyXkizPjaCMCfCPIpwRACht5OJdRH0LwEhGRhJhSHEUdh7p
 WGNEQr5XVnd2Xb+dHfpBeq6a0vPmOv8oDXzRCnGpDeTjGE3kEQy8PGeDqm4dsD+B
 wOmeCYN/NmUnurpZrW2x151bKgCbcjwiTksCsIhZmN1kRHqAR+NkoHSUOiji6INw
 LvpyU91X7ji1yyYlhPTu
 =N0P/
 -----END PGP SIGNATURE-----

updated AUTHORS

- changed "Maintained by Oliver Beckstein and Elizabeth Denning" to
  "Maintained by the MDAnalysis Core Devs Team"
- added Wouter Boomsma (for PR MDAnalysis#976)
- removed explicit mentions for KDTree (Hamelryck) because since 0.11.0
  we do no longer include the actual code (see PR MDAnalysis#395 and MDAnalysis#383)
- removed explicit mentions for helanal (Hall) because Ben Hall is
  a contributing author
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment