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

Can now initialize a universe with a Topology object #650

Merged
merged 1 commit into from
Jan 26, 2016

Conversation

dotsdl
Copy link
Member

@dotsdl dotsdl commented Jan 22, 2016

As an alternative to giving a topology file, now an existing Topology object can be given to a Universe on init. This allows many Universe objects to share the same Topology object, so changes to the topology features of one changes them all.

@dotsdl
Copy link
Member Author

dotsdl commented Jan 22, 2016

Not sure if this was the best place to put this functionality. I don't think it really hurts, though. But that's what this PR is for. :D

self.filename = None
else:
self.filename = args[0]
topology_format = kwargs.pop('topology_format', None)
Copy link
Contributor

Choose a reason for hiding this comment

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

kwargs.pop('topology_format') is enough. :D

updated: oops, mistaken with .get method.

kwargs.get('format', None) is None

should be

kwargs.get('format') is None

Copy link
Member Author

Choose a reason for hiding this comment

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

This was already here; the reason it counts as a change is because of the indentation into another if-else level. I see the point and will probably remove it, but as it is it is more explicit without looking up the default rules for dict.get().

Copy link
Contributor

Choose a reason for hiding this comment

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

but as it is it is more explicit without looking up the default rules for dict.get().

agree.

Copy link
Member

Choose a reason for hiding this comment

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

The argument should go into the definition of the init. That is much better. Kwargs should only be used to pass arguments to called functions.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kain88-de I agree, and if it were completely up to me I would make it so that the Universe init took topfile followed by trajfile, with the possibility for trajfile to be a list of filenames. Because instead we use *args style arguments for this, it's not easy to do unless we're willing to break a very visible API piece.

Copy link
Member

Choose a reason for hiding this comment

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

I think the other thing using *args lets us do easily is having no traj/all in one files (gro/pdb). Otherwise you'd need Universe(top, None) or something ugly

Copy link
Member Author

Choose a reason for hiding this comment

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

None could always be the default for traj. I don't think it would be a problem, but it would be a clear change.

@richardjgowers
Copy link
Member

I dunno, I dislike how long the current Universe init is, so I'd prefer a class method like Universe.from_Topology. Is there a reason this won't work here?

@richardjgowers richardjgowers added this to the Topology refactor milestone Jan 22, 2016
@dotsdl
Copy link
Member Author

dotsdl commented Jan 23, 2016

@richardjgowers I think the init should be broken up into methods, but that it should still be able to take Topology objects as a topologies. I think this is a good place to discuss this, as I do think the Universe init could use some rethought.

# if we're given a Topology object, we don't need to parse anything
if isinstance(args[0], Topology):
self._topology = args[0]
self.filename = None
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should remember the filename in the Topology object, so here self.filename = topology.filename. It will be annoying if somewhere down the line we've lost memory of what file this came from

@richardjgowers
Copy link
Member

Actually, with #643, it does just make sense to keep the (topology, traj) signature, even if it's a not a topology file, so I like this.

richardjgowers added a commit that referenced this pull request Jan 26, 2016
Can now initialize a universe with a Topology object
@richardjgowers richardjgowers merged commit ee8933a into issue-363 Jan 26, 2016
@richardjgowers richardjgowers deleted the issue-363-unifromtop branch January 26, 2016 10:18
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