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

Update density.py #612

Closed
wants to merge 9 commits into from
Closed

Conversation

Balasubra
Copy link
Contributor

To update atoms selection inside density_from_Universe function: a new parameter has been added ("sel_update"). When user set sel_update=True, then atomselection is updated for each frame. Accordingly lines 515 to 520 have been modified to consider/not to consider updates for each frame.

To update atoms selection inside density_from_Universe function: a new parameter has been added ("sel_update"). When user set sel_update=True, then atomselection is updated for each frame. Accordingly lines 515 to 520 have been modified to consider/not to consider updates for each frame.
@@ -511,7 +512,13 @@ def current_coordinates():
for ts in u.trajectory:
print("Histograming %6d atoms in frame %5d/%d [%5.1f%%]\r" % \
(len(coord), ts.frame, u.trajectory.n_frames, 100.0 * ts.frame / u.trajectory.n_frames),)
coord = current_coordinates()
if sel_update=True:
Copy link
Member

Choose a reason for hiding this comment

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

The syntax is not correct here. To check for equality one would use ==. But since sel_update contains a boolean that is not even necessary. Just writinge if sel_update is enough.

if sel_update:
    # do stuff

Copy link
Member

Choose a reason for hiding this comment

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

sel_update --> update_selection

@richardjgowers richardjgowers added this to the 0.13 milestone Jan 7, 2016
@orbeckst
Copy link
Member

orbeckst commented Jan 7, 2016

Your tests failed. Follow the link Details for continuous-integration/travis-ci/pr — The Travis CI build failed and fix whatever made the test fail (probably the syntax error that @kain88-de pointed out but you need to check for yourself – that's the usual development pattern: submit PR, check tests, fix, repeat).

@@ -450,6 +450,7 @@ def density_from_Universe(universe, delta=1.0, atomselection='name OH2',
cutoff
With *cutoff*, select '<atomsel> NOT WITHIN <cutoff> OF <soluteselection>'
(Special routines that are faster than the standard AROUND selection) [0]
sel_update : When set to True, atom selection is updated for each frame
Copy link
Member

Choose a reason for hiding this comment

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

sel_update --> update_selection

@Balasubra
Copy link
Contributor Author

Sorry for the delayed response. Now i corrected code taking your suggestion. I am presently checking it.

@@ -450,6 +450,8 @@ def density_from_Universe(universe, delta=1.0, atomselection='name OH2',
cutoff
With *cutoff*, select '<atomsel> NOT WITHIN <cutoff> OF <soluteselection>'
(Special routines that are faster than the standard AROUND selection) [0]
update_selection
True: Atom selection is updated for each frame
Copy link
Member

Choose a reason for hiding this comment

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

at the bottom of the doc string add

.. versionchanged:: 0.13.0
   *update_selection* keyword added

@orbeckst
Copy link
Member

orbeckst commented Jan 8, 2016

Looking good, only a few things to do:

  1. address my comment regarding the docs
  2. (optional) reqrite your history and squish the five "Update density.py" commits into one (see below)
  3. add entry to CHANGELOG (see below)
  4. add yourself as an author to MDAnalysis

History rewriting

Do you know how squish all five commits into a single one (using e.g. git rebase -i)? If not then that's ok (it's a dangerous operation...) and then let us know and one of us can do it.

The commit history in the develop branch should be reasonable clean, along the lines of "one commit, one feature" (and as little as work in progress).

Also, if you rewrite your history with rebase, make the commit title and text more expressive, along the lines of

add option to update selection every frame for density calculations

To update atoms selection inside density_from_Universe function: a new keyword
has been added ("update_selection"). When user set sel_update=True, then 
the atomselection is updated for each frame. 

Then make another commit with CHANGELOG and authorship:

CHANGELOG

  • add an entry to the 0.13.0 entry in package/CHANGELOG under Enhancements, briefly describing the change
  • add you git handle balasubra to the contributor list for 0.13.0

Authorship

This is your first contribution to MDAnalysis --- welcome and thank you!

  • add your real name to package/AUTHORS under a 2016 heading
  • add your real name to the authors list in package/doc/sphinx/source/conf.py (see comment in the file where to put your name)

I am adding you as a contributor to the project on github and you should get a notification soon.

@Balasubra
Copy link
Contributor Author

Hi,
No 1. Done
2. Merging commits and History rewriting : Could nt figure out how to do it.
3, 4 Changes in CHANGELOG, package/AUTHOURS, conf.py : All these have been inserted in Balasubra-Issue584 branch.

@orbeckst orbeckst assigned orbeckst and unassigned kain88-de Jan 8, 2016
@orbeckst
Copy link
Member

orbeckst commented Jan 8, 2016

I'll do the clean-up.

@orbeckst
Copy link
Member

orbeckst commented Jan 8, 2016

Note: This PR belongs to #584 .

@orbeckst
Copy link
Member

orbeckst commented Jan 8, 2016

This is now on PR #614

@orbeckst orbeckst closed this Jan 8, 2016
@Balasubra Balasubra deleted the Balasubra-Issue584 branch February 12, 2016 15:59
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