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

bug fixes in lib.distances #2083

Merged

Conversation

zemanj
Copy link
Member

@zemanj zemanj commented Sep 26, 2018

Fixes (partially) #2046. This is the fourth of a series of related PRs following PRs #2048, #2062, and #2070.

Changes made in this Pull Request:

  • Ensured that none of the functions in MDAnalysis.lib.distances modifies its input coordinate arrays in-place. This fixes a bug I introduced in PR Simplified code in lib.distances a bit #2048 (see updated issue MDAnalysis.lib.distances needs rework #2046).
  • Allowed for empty coordinate array input (shape=(0,3)) in all lib.distances functions.
  • Fixed failures / wrong distance output of *capped_distance() in several corner cases.
  • Added corresponding tests ensuring non-modified input, correct functionality with empty input, and correct return types.
  • Improved return type specifications in the docs.

PR Checklist

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

@codecov
Copy link

codecov bot commented Sep 26, 2018

Codecov Report

Merging #2083 into develop will increase coverage by 0.04%.
The diff coverage is 98.63%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2083      +/-   ##
===========================================
+ Coverage    89.37%   89.41%   +0.04%     
===========================================
  Files          159      159              
  Lines        18712    18731      +19     
  Branches      2684     2696      +12     
===========================================
+ Hits         16724    16749      +25     
+ Misses        1384     1381       -3     
+ Partials       604      601       -3
Impacted Files Coverage Δ
package/MDAnalysis/lib/distances.py 97.46% <98.63%> (+2.19%) ⬆️

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 8918d42...7bd977e. Read the comment docs.

@zemanj
Copy link
Member Author

zemanj commented Sep 26, 2018

This is not ready for review. Since I have to add tests for empty inputs to satisfy the diff coverage, I'll do that for all functions right away.

@richardjgowers
Copy link
Member

@zemanj how/where were coordinates being modified in place?

@zemanj zemanj changed the title fix bug introduced in PR #2048 bug fixes in lib.distances Sep 28, 2018
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 good, few comments would be nice

package/MDAnalysis/lib/distances.py Show resolved Hide resolved
package/MDAnalysis/lib/distances.py Show resolved Hide resolved
@zemanj
Copy link
Member Author

zemanj commented Sep 28, 2018

@richardjgowers

@zemanj how/where were coordinates being modified in place?

This happened when I introduced the new @check_coords() decorator. For some functions, I falsely assumed that they either make copies of (or only read from) the input arrays internally. I fixed the problem by removing the enforce_copy=False decorator keyword in these cases. In order to guarantee unmodified input arrays, I added the class TestInputUnchanged to lib/test_distances.py.

@zemanj
Copy link
Member Author

zemanj commented Sep 28, 2018

@richardjgowers I added the requested comments. Moreover, I removed the obsolete dtype conversion in the return statements of capped_distance() and self_capped_distance(). This has the advantage that the return type is now also tested for the individual methods (not really required IMHO, but nice to have).

@zemanj
Copy link
Member Author

zemanj commented Sep 29, 2018

@richardjgowers Do you want to take another look or shall I merge?

@richardjgowers richardjgowers merged commit edb1643 into MDAnalysis:develop Sep 29, 2018
@zemanj
Copy link
Member Author

zemanj commented Sep 29, 2018

@richardjgowers Thank you for reviewing!

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.

2 participants