Skip to content

enforced double precision calculations in *Group.center()#1936

Merged
richardjgowers merged 3 commits intoMDAnalysis:developfrom
zemanj:group_center
Jun 27, 2018
Merged

enforced double precision calculations in *Group.center()#1936
richardjgowers merged 3 commits intoMDAnalysis:developfrom
zemanj:group_center

Conversation

@zemanj
Copy link
Member

@zemanj zemanj commented Jun 12, 2018

Fixes #1930 (at least in part)

Changes made in this Pull Request:

  • enforced calculations in *Group.center() to be performed in double precision
  • for compatibility reasons, added automatic type conversion to dtype=np.float32 for input coordinate arrays of lib.distances.apply_PBC(), lib.distances.distance_array(), and lib.distances.self_distance_array()
  • arrays returned by *Group.center() will be of dtype=np.float64 unless (compound != 'group' and pbc==True). This exception could be eliminated by providing a double-precision version of lib.distances.apply_PBC().

PR Checklist

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

@richardjgowers
Copy link
Member

@zemanj if you're doing a few you may as well do all the functions in distances (eg calc_bonds)

@coveralls
Copy link

coveralls commented Jun 12, 2018

Coverage Status

Coverage increased (+0.008%) to 89.801% when pulling de3deb7 on zemanj:group_center into 70b9e9f on MDAnalysis:develop.

@richardjgowers richardjgowers self-assigned this Jun 12, 2018
@zemanj
Copy link
Member Author

zemanj commented Jun 13, 2018

@richardjgowers I only did the ones that were needed to pass the tests since I didn't know whether you guys are ok with what I'm doing. Sure I can do that for the remaining functions as well.
Updates for tests, docs, and changelog are also needed.

@orbeckst
Copy link
Member

@richardjgowers did you already take this PR into account in your PR #1939?

If not, should we merge this one "as is" and then worry about double precision in your PR (rebased) or do a new one to add remaining functionality?

Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs typecasting on all accessible functions in lib.distances

@richardjgowers
Copy link
Member

@orbeckst my PR just pushes single angle/etc calculations into the existing functions. This PR makes those existing functions work with double precision arguments. So they're complimentary and non conflicting

@zemanj
Copy link
Member Author

zemanj commented Jun 27, 2018

Recent changes:

  • Added typecasting of input coordinate arrays to np.float32 for all accessible functions in lib.distances
  • Changed docstrings accordingly
  • Removed respective "wrong dtype" tests, parametrized tests for np.float32 and np.float64 instead.

Open questions:

  • Do we need tests for other dtypes?

Will add CHANGELOG entry once this passes.

@richardjgowers
Copy link
Member

@zemanj I think float32 & 64 is enough

Copy link
Member

@richardjgowers richardjgowers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks awesome!

@richardjgowers richardjgowers merged commit 36a7a5a into MDAnalysis:develop Jun 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants