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

calculates distances using MDA.distance_array #31

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

joaomcteixeira
Copy link
Owner

@joaomcteixeira joaomcteixeira commented Mar 24, 2020

Addresses #30 using MDA.distance_array

@richardjgowers could you review this change and let me know if the implementation is correct according to MDAnalysis functionalities?

Regarding the unwrap and center_of_mass issue, I have addressed this in the documentation. For now, I think this CLI should be agnostic to that because there are other CLIs that provide that functionality. Let me know if you think otherwise.

Thank you

@joaomcteixeira joaomcteixeira added the enhancement New feature or request label Mar 24, 2020
@joaomcteixeira joaomcteixeira self-assigned this Mar 24, 2020
@codecov-io
Copy link

codecov-io commented Mar 24, 2020

Codecov Report

Merging #31 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #31   +/-   ##
=======================================
  Coverage   98.76%   98.77%           
=======================================
  Files          21       21           
  Lines         974      977    +3     
  Branches       82       82           
=======================================
+ Hits          962      965    +3     
  Misses         12       12           
Impacted Files Coverage Δ
src/taurenmd/cli_distances.py 100.00% <100.00%> (ø)

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 58529cc...3a0290b. Read the comment docs.


### Note on Periodic Boxes

The distances calculated with this client do not account for artifacts

Choose a reason for hiding this comment

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

Is the idea here that users would have to create a new trajectory of unwrapped positions before they can proceed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

That is how I am using it, and my original idea. I have improved the description in the CLI docs (last commit). Having these two steps separate forces the users to have a granular comprehension and understanding of their operations. So I will keep it this way in this PR but I may (almost certainly) return to this in the future. Thanks!

@richardjgowers
Copy link

@joaomcteixeira I've not had time to run this myself, but it looks correct.

@joaomcteixeira
Copy link
Owner Author

Thanks @richardjgowers for your comments and help. As said in the comment I have improved the description of the CLI. I will wait sometime before merging this PR in case you want to make any additional comments.

Again, thanks for you feedback and help!

@joaomcteixeira
Copy link
Owner Author

Travis-CI passes but I don't know why it does not communicate here...

@codeclimate
Copy link

codeclimate bot commented Nov 19, 2021

Code Climate has analyzed commit c0f51c9 and detected 0 issues on this pull request.

View more on Code Climate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants