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

Review of transformations module (for Py3k) #401

Closed
richardjgowers opened this issue Aug 24, 2015 · 3 comments
Closed

Review of transformations module (for Py3k) #401

richardjgowers opened this issue Aug 24, 2015 · 3 comments

Comments

@richardjgowers
Copy link
Member

Part of Issue #260

The lib.transformations module is actually a C extension that lives in src/transformations/transformations.c. I'd initially thought that this would require a rewrite to get working with Python 3, but the header of the C file says it works with 3.1. There's a newer version of this extension which looks like it supports 2.7/3.4

http://www.lfd.uci.edu/~gohlke/code/transformations.c.html
http://www.lfd.uci.edu/~gohlke/code/transformations.py.html

There's also the line of thought that if we could scrap this module and replace it with numpy/scipy then that would simplify the package as a whole? Obviously all functions would need a counterpart, and performance would need to be checked.

Points for discussion:

  • will it work with Python 3 as is?
  • is it worth updating it? (Will this break previous usage of it?)
  • is it worth keeping? (What does this give that scipy/numpy doesn't?)
@kain88-de
Copy link
Member

Why not switch to the separately packaged transforms3d?

https://github.com/matthew-brett/transforms3d

it is installable with pip. As far as I see it only depends on numpy. Has good coverage. It is pure python. Works from 2.6 - 3.5. And it is mentioned as an alternative in the module you give (it looks like it is actually derived from it)

@orbeckst
Copy link
Member

Main concerns for external code are

  1. runs correctly,
  2. has tests,
  3. decent performance,
  4. API is stable.

I think as long as we can migrate transformations.c to Py3 we should just keep it as it is. I think there are more exciting areas in MDAnalysis to work on and it does the job it is supposed to. I rather have old (and tested!) code included, which is guaranteed to never break the API than chasing the latest and greatest for possibly very small returns. I always fear external dependencies changing their API and then we either have to chase them (and that's not always easy in an open source project with volunteers dedicating time) or having to pin releases to certain "last working" releases of dependencies (which gets you deeper into dependency hell and mandatory virtual envs).

@richardjgowers
Copy link
Member Author

Hmm, the module looks good but it looks like they've only ported the pure python half of transformations, which might impact the performance. Their test coverage is 98% though.... which tempts me to appropriate their tests for our version of the same module though.........

Ok, I think this will work with Py3, so I'm going with @orbeckst 's idea of spending effort elsewhere, if it doesn't we can return to this

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

No branches or pull requests

3 participants