-
Notifications
You must be signed in to change notification settings - Fork 664
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
Deprecations on all components to disappear or change in Topology Refactor #599
Comments
Ahh this is a good point. We'll have to start a list on the wiki or something... once everything is finalised |
Wiki/Issue363-Changes by @richardjgowers :
|
@orbeckst Yes. If you're using Residues, ResidueGroups Segments or SegmentGroups as AtomGroups, (accessing atom level properties such as positions off them) then yes it will break things. WRT #411, an XGroup now always returns things of len(XGroup), with the exception being
|
@richardjgowers @dotsdl the changes seem to be very nice. For the changes page. Could you maybe create some real-code example how things will change from the current Topology Model to the new Model. I also would'nt mind if you start doing that in a notebook that you upload and link somewhere (a gist or maybe already as a new blog-post). |
Correct. Doing
will give an
The same principle applies for |
I've been thinking about this, and it's a little complicated to add in deprecation warnings, but I think it's possible. So for example, But if we made a class AtomGroup
# mostly unchanged
class DeprecatedAtomGroup(AtomGroup)
# add in some magic here to give deprecation warnings
# maybe even hacking into getattr.......
class Residue(DeprecatedAtomGroup)
class RG(DeprecatedAtomGroup) |
On 5 Jan, 2016, at 05:35, Richard Gowers wrote:
There's still @kain88-de's valid question as to should we deprecate if we cannot provide an alternative yet. I think the answer is that this will be a second API break and we need to think about how to mitigate this. The last break already annoyed users, as I was being told. |
Question about
Are these groups unordered or ordered, i.e. like real sets (random order) or lists with unique elements in defined order? |
I don't think @dotsdl and I really 100% decided on the set idea, it just seemed like the neatest solution. So considering a trivial system with 4 atoms 2 residues:
I think upshifts should (and currently do?) return a set:
It's the downshifts that are a little confusing, so this seems clear
But then others are more confusing
Or more generically:
None of this will require much work to change on the new branch, so a discussion is welcome! |
The current implementation does something like RG.atoms = set(R.atoms for R in RG) I also wondered if you would want duplicate atoms for EDIT P.S.: If sets then I would want ordered sets, i.e. atoms in ascending atom index order. |
They are always ordered by |
To elaborate further on the why... Making
If one wants the resids of the residues represented among the atoms in the
Likewise, if one wants an
The point is, making |
TLDR: Any other scheme requires probably different rules for what you get depending on which level you are coming from, which gets confusing and unintuitive real fast. We considered not using sets for about a day before we realized other schemes were hard to explain even to ourselves. |
On 5 Jan, 2016, at 12:47, David Dotson wrote:
I am not so sure that I like the fact that you can have ag = u.atoms[[0,1,0]] You might break code in rather subtle ways. |
On 5 Jan, 2016, at 12:50, David Dotson wrote:
Fair enough, but I think the discussion here really shows that we have to be careful how we introduce this and how we prepare users. It's all good to be excited about a sleek new implementation (which is great, don't get me wrong!) but you also have a responsibility towards the user base � and most of them hate having to debug code that "used to work". Thus, it should be very clear what changes (and the wiki page is a starting point). I also liked the idea of writing a blog post before a merge, something like a vision for what needs to be changed and why and explain some of the rationale. |
What is the point of |
Because the underlying principle was "everything is an AtomGroup", it was very convenient to either use the object, let's call it What your are proposing for I am not saying that we can't change this but one has to be a bit sensitive to current usage. |
How do you say an atom is unique? Do you go with the ID of the atom? I still think it would be very nice to have an example where old code has to be changed or would be broken by the changes. That would make it easier to see how the changes would affect users. |
@kain88-de yeah, an Atom is just an index. Or rather, all Atoms/Residues/Segments are uniquely defined according to (Universe, Level (A/R/S), Index). The class defines the first two, so within the class you just worry about indices. |
@orbeckst I think there are a few possible options for this whole scheme we can debate over to help focus discussion 1. Everything gives a setSame rule for everything, in which a call to
2. Everything gives a set except for the same levelSame as 1, with the exception that at the same level you get back the same object. Calling
3. Order and repeat elements preserved as much as possibleAdds At the At the At the
4 Upwards give sets, downwards gives list comprehensionsSimilar to 3 except Giving Adds 1's downwards rules can be done using
|
I'm an advocate for 1, although the others can technically work. |
The main difference between 1 and 2 is that, in 1, To what extent are I foresee less surprise with 1 as we can say that these properties always return a new object ans that they always deduplicate elements. I am curious about 3. Is it possible to get (even not directly), with 1, what 3 would output? |
@jbarnoud yes I think you're correct about 1 vs 2 What I don't like in 1 is that I think I'd like to invent 4, which is identical to 3 except |
I'll concede to just skimming this issue, but do let me know if I need to write any lib2to3 fixers for API changes. Extending ten2eleven to provide warning messages where the deprecation architecture is complicated or philosophically not correct might be an idea. |
And what does segment do? Why not only atoms and residues? |
On 12 Feb, 2016, at 17:31, Hai Nguyen wrote:
Larger units, such as, say actual molecules. Atoms are real, molecules are real, residues are convenient ;-) (But of course, the whole solvent or the membrane could also conveniently be grouped as a segment, so that's why we are not calling it a molecule... that's what fragments are for, but they don't fit into the hierarchy.) |
@hainm our new topology system is more performant, both in speed and memory usage, and more flexible (no hardcodes of attributes) than either the current MDAnalysis topology system or that of mdtraj. There is no reason to regress on this front. The new system also preserves much of the existing intuitive API of MDAnalysis that works quite well, but internally eliminates the possibility of staleness along with the performance benefits. The only things that need deciding are what attribute access down the hierarchy returns. See the conversation earlier and my gist on the question. |
So to sidestep the ambiguity of accessing lower-level attributes from a single object ( Instead, accessing Is this a satisfying resolution? In short, we leave out lower-level properties from |
Ah, should have re-read your summary:
You're in favor of making |
yeah, I only make comment based on what I look at MDA and mdtraj tutorials. So I don't know what users want to use. Can you propose a detail example about new topology? Whether the new design will be widely used from users? How often it is used? I am afraid that we are trying to introduce many idea but ending no one ever uses (I throwed away 1/2 pytraj code that I wrote for 1/2 year to get simplicity :D). |
For example: how often people use '_setter' method for |
The new topology system works under the hood. Most users won't notice it, save for the performance gains and the API breaks here and there (which we are discussing now, in this very thread, and deprecating ahead of time what we can). See the wiki for some details on what's changed and why. More will be written about it as we get closer to the merge of |
Huh? I have no idea what you're asking. At any rate, this thread is pretty focused on a particular question, and I'd rather we keep it focused on that question until it's resolved. You are welcome to have a look at the |
thanks. sorry to dilute this issue. |
Hey I think the nested returns are new since we last spoke, but I think they make sense. The only problem I see with them is that some atom properties don't work like that (mass and charge) and so it's not completely smooth. Ie RG.Atomprop isnt always always the same shape (but is always the same len or shape[0]) |
@dotsdl ,
Yes, together with your proposal to use nested lists (or iterables) where appropriate. It is true that this breaks orthogonality (as @richardjgowers pointed out) but I think we can gain more in functionality. At a minimum we do guarantee that for plural attributes one always gets an iterable of |
@orbeckst given your comment here, I take it this means we're fine with leaving out things like |
Yes. Oliver Beckstein Am Mar 10, 2016 um 19:18 schrieb David Dotson [email protected]:
|
Excellent. There's a fairly clear path forward then. Will keep hammering to get everything in the right place. As I said, I think we're at least 90% there already. |
I'm also cool with that, but what is then the recommended way to have a list of all residue names? (Unduplicated, one name per residue) |
@mnmelo no matter what group you're using to start with, you'd do |
Ok, suspected as much. Cool with me. |
@dotsdl if you had a RG, doesn't |
@richardjgowers ah yeah, fair enough. doing @mnmelo if you want resnames for each residue but with absolutely no duplicate residues, you would do |
This in accordance with the consensus on #599 (#599 (comment)).
Once #698 is merged, I'll fill in the checklist and we'll close. :D |
The 0.16 release features a complete overhaul of the topology system, giving better performance and consistency in a major part of MDAnalysis' core functionality. This also includes the disappearance of some API elements that no longer made sense; these must be marked as deprecated in the 0.15 release.
Add to this list, and check when marked deprecated in
develop
branch.Deprecate and recommend alternative:
AtomGroup.get_*
gettersAtomGroup.set_*
settersResidueGroup.get_*
gettersResidueGroup.set_*
settersSegmentGroup.get_*
gettersSegmentGroup.set_*
settersDeprecate and indicate workaround (eg use RG.atoms.names)
Residue
as AtomGroupResidueGroup
as AtomGroupSegment
as AtomGroupSegmentGroup
as AtomGroupIndicate when the deprecation will happen
The text was updated successfully, but these errors were encountered: