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

A Universe now holds onto its initialization kwargs. #813

Merged
merged 8 commits into from
Apr 6, 2016

Conversation

dotsdl
Copy link
Member

@dotsdl dotsdl commented Apr 3, 2016

Fixes #292

Changes made in this Pull Request:

  • Universe holds onto a copy of its given keyword arguments on init.

PR Checklist

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

This is useful for external libraries, such as MDSynthesis
(https://github.com/datreant/MDSynthesis), to re-initialize a Universe
from its arguments.

This is useful for external libraries, such as MDSynthesis
(https://github.com/datreant/MDSynthesis), to re-initialize a Universe
from its arguments.
@dotsdl dotsdl added this to the 0.15.0 milestone Apr 3, 2016
@dotsdl
Copy link
Member Author

dotsdl commented Apr 3, 2016

The smallest PR you will ever merge.

@kain88-de
Copy link
Member

If you add a test I might consider reviewing this gigantic PR 😉

Basic test that it holds on to these as we'd expect in
test_atomgroup.TestUniverse. Another test that these work as expected in
guess_bonds tests.
@dotsdl
Copy link
Member Author

dotsdl commented Apr 3, 2016

Added some tests; see what you think. It's worth discussing if this should be an underscore attribute or whether it should be exposed (as a read-only property like Universe.kwargs).

u2 = MDAnalysis.Universe(u.filename, u.trajectory.filename,
**u._kwargs)

assert_(u2._kwargs['fake_kwarg'] is True)
Copy link
Member

Choose a reason for hiding this comment

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

That looks good to me. Would it be to hard to compare the complete dictionaries or u and u2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not at all. I'll add that in, too.

@@ -1724,7 +1724,19 @@ def test_set_dimensions(self):
u.dimensions = np.array([10, 11, 12, 90, 90, 90])
assert_allclose(u.dimensions, box)

def test_universe_kwargs(self):
Copy link
Member

Choose a reason for hiding this comment

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

QC has a good point here making this a staticmethod

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we don't use self anywhere inside? Many of the other methods don't use self anywhere inside either.

Copy link
Member

Choose a reason for hiding this comment

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

I like QC's rationale for making it @staticmethod:

"This will improve performance and make it clear to users of your code that the function does not use any attributes of the class."

We should do it when we can.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, done.

@kain88-de
Copy link
Member

Looks good to me as soon as the tests go through.

@@ -2121,11 +2133,13 @@ def _check_universe(self, u):
assert_equal(len(u.atoms[3].bonds), 2)
assert_equal(len(u.atoms[4].bonds), 1)
assert_equal(len(u.atoms[5].bonds), 1)
assert_('guess_bonds' in u._kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Why does it matter if this kwarg is saved? The previous test should have covered this

Copy link
Member Author

Choose a reason for hiding this comment

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

The previous tests with a nonexistent kwarg. This is to test that we hold on to kwargs even after init is finished, which should be guaranteed since we do a deep copy of the kwarg dict. This was a convenient place to slot in a check on that.

@dotsdl
Copy link
Member Author

dotsdl commented Apr 4, 2016

I'm going to expose the stored kwargs with a read-only property Universe.kwargs. I don't think there's any downside to this, and it makes it so that external tools don't need to use an underscored attribute, which should be discouraged.

@dotsdl
Copy link
Member Author

dotsdl commented Apr 4, 2016

Ready for merge if all is good.

@@ -2136,6 +2150,8 @@ def test_universe_guess_bonds_with_vdwradii(self):
u = MDAnalysis.Universe(two_water_gro_nonames, guess_bonds=True,
vdwradii=self.vdw)
self._check_universe(u)
assert_(u.kwargs['guess_bounds'] is True)
Copy link
Member

Choose a reason for hiding this comment

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

bounds

@dotsdl
Copy link
Member Author

dotsdl commented Apr 4, 2016

Ah, sorry, really off my game today. Thanks @richardjgowers !

@orbeckst
Copy link
Member

orbeckst commented Apr 5, 2016

Will u.kwargs ever hold other "complicated" objects, namely, will this be a problem for serializing? I suppose we can cross these bridges when we reach them...

@dotsdl
Copy link
Member Author

dotsdl commented Apr 5, 2016

@orbeckst u.kwargs will hold whatever is given to it; it's up to other libraries/users to decide what they do with them. The way MDSynthesis handles this is to throw an exception if kwargs are added that have non JSON-serializable values.

@dotsdl dotsdl mentioned this pull request Apr 5, 2016
6 tasks
@@ -1724,7 +1724,20 @@ def test_set_dimensions(self):
u.dimensions = np.array([10, 11, 12, 90, 90, 90])
assert_allclose(u.dimensions, box)

@staticmethod
def test_universe_kwargs(self):
Copy link
Member

Choose a reason for hiding this comment

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

you also need to remove the self variable.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Sorry!

@dotsdl
Copy link
Member Author

dotsdl commented Apr 5, 2016

Okay, my push isn't showing up. Not sure what's going on, here.

@kain88-de
Copy link
Member

github has some problems right now.

@dotsdl
Copy link
Member Author

dotsdl commented Apr 6, 2016

Looks like it finally made it, and the tests pass: https://travis-ci.org/MDAnalysis/mdanalysis/builds/120977104

@kain88-de kain88-de merged commit fdd5dca into develop Apr 6, 2016
@orbeckst orbeckst deleted the feature-universe_store_kwargs branch April 11, 2016 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants