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.

Sign up for GitHub

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

Jump to bottom

wateranalysis: fix for empty selections and PEP8 #1323

Merged
merged 2 commits into from
May 1, 2017
Merged

wateranalysis: fix for empty selections and PEP8 #1323

merged 2 commits into from
May 1, 2017

Conversation

orbeckst
Copy link
Member

This is @alejob 's PR #1322 after rebase against develop and history-condensing

Changes made in this Pull Request:

  • fix wateranalysis for the case when selections are empty
  • PEP 8 formatting

This PR supersedes #1322 and closes #1322.

PR Checklist

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

@orbeckst orbeckst requested a review from alejob April 28, 2017 05:37
@orbeckst
Copy link
Member Author

Note: This needs to be merged (don't do anything with rebase because otherwise the author information will get messed up).

@orbeckst
Copy link
Member Author

@alejob I rewrote the history of your PR to format it as two nice separate commits:

  1. fix of the bug together with test case
  2. PEP 8 formatting

Please check that everything looks ok.

I did minor orthography fixes where I found them. I also reverted a few PEP8 changes in the specific cases of division and comma, e.g.

val0 / n, val1 / n

to

val0/n, val1/n

roughly following PEP: whitespace: Other Recommendations... and simple aesthetics.

@orbeckst
Copy link
Member Author

orbeckst commented Apr 28, 2017 •
edited
Loading

Sorry, QC suggested to make lg2() a @staticmethod, so I rebased and force-pushed.

@orbeckst orbeckst mentioned this pull request Apr 28, 2017
Closed
4 tasks
@orbeckst orbeckst added this to the 0.16.x milestone Apr 28, 2017
alejob
alejob reviewed Apr 28, 2017
View reviewed changes
package/MDAnalysis/analysis/waterdynamics.py
return selection

# Second Legendre polynomial
lg2 = lambda self,x : (3*x*x - 1)/2
@staticmethod
Copy link
Member

Choose a reason for hiding this comment

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

It makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

All seems good ;)

@orbeckst
Copy link
Member Author

Spurious travis failure, so I restarted the job https://travis-ci.org/MDAnalysis/mdanalysis/jobs/226689879

@orbeckst
Copy link
Member Author

@alejob , once travis completes successfully, your or I can merge the PR.

alejob
alejob approved these changes Apr 28, 2017
View reviewed changes
Copy link
Member

@alejob alejob left a comment

Choose a reason for hiding this comment

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

All ok to merge.

@alejob
Copy link
Member

alejob commented Apr 28, 2017

I can't merge the PR, I'm not allowed.

@orbeckst
Copy link
Member Author

We need another review from an @MDAnalysis/coredevs .

At least one approved review is required by reviewers with write access.

(I can't merge it either.)

alejob added 2 commits April 29, 2017 09:43
@alejob @orbeckst
wateranalysis: Fixed case when no water molecules in selection.
20ccd1c 
- wateranalysis could not handle empty selections: now we check
  explicitly and return 0 or (0, 0, 0) when necessary
- Improved print to screen format; now uses ProgressMeter
- Avoiding hidden return inside if/else (improves code readability)
  and made lg2(x) a static method
- Added test
- Fixed some orthography.
- CHANGELOG updated.
@alejob @orbeckst
wateranalysis: Formatted according to PEP8
3c8e31f 
- Adapted to PEP8, not fully, but most
- CHANGELOG formatting fixes
@orbeckst
Copy link
Member Author

I manually rebased against develop and force-pushed.

@richardjgowers richardjgowers self-assigned this May 1, 2017
@richardjgowers richardjgowers merged commit 5c6700b into develop May 1, 2017
@richardjgowers richardjgowers deleted the PR-1322-alejob branch May 1, 2017 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardjgowers richardjgowers richardjgowers approved these changes

@alejob alejob alejob approved these changes

Assignees

@richardjgowers richardjgowers

Labels
Component-Analysis needs review
Projects
None yet
Milestone
0.16.x
Development

Successfully merging this pull request may close these issues.
3 participants
@orbeckst @alejob @richardjgowers