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

add removeAtom, removeResidue, removeChain, removeBond to app.Topology #41

Closed
rmcgibbo opened this issue Jul 3, 2013 · 11 comments
Closed

Comments

@rmcgibbo
Copy link
Contributor

rmcgibbo commented Jul 3, 2013

Would other people use these methods? I find myself needing to implement these methods right now for my project -- if this is functionality that other people would use, it might be good to put it into the python app.

@tjlane: you were implementing this for your evaporated water droplets too, if I remember correctly?

@peastman
Copy link
Member

peastman commented Jul 3, 2013

I think Modeller.delete() is exactly what you're looking for.

@rmcgibbo
Copy link
Contributor Author

rmcgibbo commented Jul 3, 2013

I want to remove atoms from a topology that I have no positions for. I guess I could give fake positions to modeller, but that seems silly.

@peastman
Copy link
Member

peastman commented Jul 3, 2013

Modeller doesn't care what positions you give it. It edits the topology and positions at the same time, because often you do have positions you need to keep in sync. But if they're fake positions that you'll never look at, it doesn't care.

@jchodera
Copy link
Member

jchodera commented Jul 3, 2013

I think I would use these methods. Sometimes we need to manipulate topologies without necessarily having coordinates.

Alternatively, we could propose that Modeller add a method to build coordinates from any existing coordinate data and idealized internal coordinates.

@jchodera
Copy link
Member

jchodera commented Jul 3, 2013

Modeller doesn't care what positions you give it. It edits the topology and positions at the same time, because often you do have positions you need to keep in sync. But if they're fake positions that you'll never look at, it doesn't care.

If the positions are irrelevant in making these operations, why shouldn't Topology also support these kinds of manipulations?

@rmcgibbo
Copy link
Contributor Author

rmcgibbo commented Jul 3, 2013

It's definitely possible to access this functionality now via Modeller -- but IMO it would be nice to have the symmetry of the remove methods, since Topology has addAtom, addResidue, etc.

@tjlane
Copy link

tjlane commented Jul 3, 2013

I'll also use whatever comes out of this. Agree w/ @rmcgibbo that having more finely divided methods is a bit more intuitive. That said I think it's a small deal!

Good to know about Modeller.delete()

@peastman
Copy link
Member

peastman commented Jul 3, 2013

If the positions are irrelevant in making these operations, why shouldn't Topology also support these kinds of
manipulations?

We can't actually remove pieces from an existing Topology. That would break all sorts of things. For example, every atom, chain, or residue has an index field that would no longer be valid. That's why Modeller creates a new Topology for every editing call, leaving the old one unchanged.

@kyleabeauchamp
Copy link
Contributor

But we could have member functions that return a new topology. They could
accept coordinates as an optional input as well...

@peastman
Copy link
Member

peastman commented Jul 3, 2013

Yes, but why would we want two completely independent but parallel topology editing facilities? That would just add complexity and create confusion. What problem is this supposed to solve?

Also, can you imagine the confusion that would ensue from having a removeAtom() method on a Topology object that actually leaves the object unchanged, returning a new object? That's certainly not what I would expect a method like that to do.

@rmcgibbo
Copy link
Contributor Author

rmcgibbo commented Jul 4, 2013

@peastman: It would be straightforward to maintain the invariants when removing an atom. You would have obviously have to reindex the remaining atoms. It is not impossible. But clearly returning a new topology is the wrong solution -- that's what modeller is for.

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

No branches or pull requests

5 participants