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

Remove instant selectors #1377

Open
richardjgowers opened this Issue Jun 2, 2017 · 12 comments

Comments

5 participants
@richardjgowers
Member

richardjgowers commented Jun 2, 2017

Another issue from the dev meetings, @orbeckst proposed removing these selectors

I'll try and summarise the currently available operations and what they do, along with the best alternative

u = mda.Universe()

# AtomGroup
# attribute based
u.atoms.H
u.atoms.select_atoms('name H')
# getitem based
u.atoms['H']
u.atoms.select_atoms('name H')

# ResidueGroup
rg = u.residues
rg.LYS
rg[rg.resname == 'LYS']

# Segment
u.segments[0].r1  # gives first residue
u.segments[0].residues[0]

# Universe
u.SYSTEM  # based on segids in topology
u.segments[u.segments.segids == 'SYSTEM']

They're also a little weird in that they *usually return Groups, but sometimes return singular objects if only one object was found. Ie RG.LYS will return a ResidueGroup unless there's only one LYS, in which case you get a Residue

Should we ditch (all of?) these?

@richardjgowers

This comment has been minimized.

Show comment
Hide comment
@richardjgowers

richardjgowers Jun 2, 2017

Member

I don't like the __getattr__ hack versions, these are weird (in terms of other packages don't do similar hacks?) and are also annoying to program around. Attribute access should be for topology items and methods.

Having looked at the __getitem__ versions though, I do sort of like these. Slicing implies selection, so why not have slicing with a string lead to a selection being made? I think there's also precedent with packages like Pandas taking advantages of this.

I would even go a little further and say, why not make __getitem__ also alias to select_atoms, ie AtomGroup['resname LYS'] == AtomGroup.select_atoms('resname LYS'). This is a little messy if we keep the existing getitem shortcuts as we have to determine what type of string we've been passed.

tl;dr

  1. ditch u.atoms.H u.s4AKE et al
  2. keep u.atoms['H']
  3. extend to u.residues['LYS']?
  4. add u.atoms['around 2.0 protein'] ?
  5. if __getitem__ == select_atoms, maybe ditch 2) & 3)
Member

richardjgowers commented Jun 2, 2017

I don't like the __getattr__ hack versions, these are weird (in terms of other packages don't do similar hacks?) and are also annoying to program around. Attribute access should be for topology items and methods.

Having looked at the __getitem__ versions though, I do sort of like these. Slicing implies selection, so why not have slicing with a string lead to a selection being made? I think there's also precedent with packages like Pandas taking advantages of this.

I would even go a little further and say, why not make __getitem__ also alias to select_atoms, ie AtomGroup['resname LYS'] == AtomGroup.select_atoms('resname LYS'). This is a little messy if we keep the existing getitem shortcuts as we have to determine what type of string we've been passed.

tl;dr

  1. ditch u.atoms.H u.s4AKE et al
  2. keep u.atoms['H']
  3. extend to u.residues['LYS']?
  4. add u.atoms['around 2.0 protein'] ?
  5. if __getitem__ == select_atoms, maybe ditch 2) & 3)
@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Jun 2, 2017

Member

if getitem == select_atoms, maybe ditch 2) & 3)

I like this

Member

kain88-de commented Jun 2, 2017

if getitem == select_atoms, maybe ditch 2) & 3)

I like this

@mnmelo

This comment has been minimized.

Show comment
Hide comment
@mnmelo

mnmelo Jun 2, 2017

Member

+1 for 1. and 5.
I also dislike __getattr__ because it can make for cryptic bugs instead of plain AttributeErrors. But I recall @orbeckst was keen on keeping the quick accessors. Change of mind?

Member

mnmelo commented Jun 2, 2017

+1 for 1. and 5.
I also dislike __getattr__ because it can make for cryptic bugs instead of plain AttributeErrors. But I recall @orbeckst was keen on keeping the quick accessors. Change of mind?

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Jun 2, 2017

Member
Member

orbeckst commented Jun 2, 2017

@richardjgowers richardjgowers referenced this issue Jun 4, 2017

Closed

add deprecation warnings for API changes #1383

3 of 3 tasks complete
@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Jun 7, 2017

Member

I would really just ditch everything (i.e., 1); I wouldn't bother with __getitem__ access, just do selections via selection strings (or boolean selections/slicing). If, over time, we find that we need to improve how we do selections or want to look more like pandas then we can re-introduce something. But initially I'd start with something simpler.

(I admit that there's a certain logic to atoms["atomname"] and residues["resname"] and segments["segname"] but it seems like an another exception that you have to teach to users. Maybe leave it sit for a while to see how it feels after simplifying the system.)

Member

orbeckst commented Jun 7, 2017

I would really just ditch everything (i.e., 1); I wouldn't bother with __getitem__ access, just do selections via selection strings (or boolean selections/slicing). If, over time, we find that we need to improve how we do selections or want to look more like pandas then we can re-introduce something. But initially I'd start with something simpler.

(I admit that there's a certain logic to atoms["atomname"] and residues["resname"] and segments["segname"] but it seems like an another exception that you have to teach to users. Maybe leave it sit for a while to see how it feels after simplifying the system.)

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Jun 13, 2017

Member

Totally random comment: should we shorten group.select_atoms() to group.select()? There is no select_residues (and I don't want one either) so it seems very superfluous to have "atoms" in the name.

Member

orbeckst commented Jun 13, 2017

Totally random comment: should we shorten group.select_atoms() to group.select()? There is no select_residues (and I don't want one either) so it seems very superfluous to have "atoms" in the name.

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Jun 13, 2017

Member

@MDAnalysis/coredevs we need a few more opinions here on how to proceed. Take your picks 1 ... 5.

Member

orbeckst commented Jun 13, 2017

@MDAnalysis/coredevs we need a few more opinions here on how to proceed. Take your picks 1 ... 5.

@richardjgowers

This comment has been minimized.

Show comment
Hide comment
@richardjgowers

richardjgowers Jun 13, 2017

Member

@orbeckst ResidueGroup.select() is a little ambiguous as to what it selects, RG.select_atoms makes it clear you'll get atoms not residues.

I can raise a new issue for __getitem__ == select_atoms, in which case we can ditch all the fancy shortcuts above.

Member

richardjgowers commented Jun 13, 2017

@orbeckst ResidueGroup.select() is a little ambiguous as to what it selects, RG.select_atoms makes it clear you'll get atoms not residues.

I can raise a new issue for __getitem__ == select_atoms, in which case we can ditch all the fancy shortcuts above.

@jbarnoud

This comment has been minimized.

Show comment
Hide comment
@jbarnoud

jbarnoud Jun 13, 2017

Contributor

+1 for 1. Why not 5, but it does not bring much except maybe confusion; now we can say that AG can be indexed like a numpy array, with the select string in __getitem__ we have to add a caveat.

select_atoms is much more explicit than select.

Contributor

jbarnoud commented Jun 13, 2017

+1 for 1. Why not 5, but it does not bring much except maybe confusion; now we can say that AG can be indexed like a numpy array, with the select string in __getitem__ we have to add a caveat.

select_atoms is much more explicit than select.

@orbeckst orbeckst removed the proposal label Jun 15, 2017

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Jun 15, 2017

Member

@jbarnoud has a good point regarding how we explain slicing of groups: being able to say "just like numpy arrays" is simple and easily understood.

Let's go with 1 (remove it all).

(The majority is also in favor of keeping select_atoms() so no changes there.)

Member

orbeckst commented Jun 15, 2017

@jbarnoud has a good point regarding how we explain slicing of groups: being able to say "just like numpy arrays" is simple and easily understood.

Let's go with 1 (remove it all).

(The majority is also in favor of keeping select_atoms() so no changes there.)

orbeckst added a commit that referenced this issue Jun 15, 2017

add deprecation warnings for API changes
- closes #1383
- included deprecations:
  - #1373 Timeseries (targeted for 0.17)
    Note that the deprecation for core.Timeseries will always show up;
    this is deliberate so that users WILL see it as it will be gone in
    the next release!
  - #1377 Quick selectors (target 1.0)
  - #782 flags (target 1.0)
- updated CHANGELOG

@orbeckst orbeckst referenced this issue Jun 15, 2017

Merged

add deprecation warnings for API changes #1403

4 of 4 tasks complete
@kain88-de

This comment has been minimized.

Show comment
Hide comment
@kain88-de

kain88-de Jun 16, 2017

Member

We should also ditch option 2 then.

Member

kain88-de commented Jun 16, 2017

We should also ditch option 2 then.

@orbeckst

This comment has been minimized.

Show comment
Hide comment
@orbeckst

orbeckst Jun 16, 2017

Member

We should also ditch option 2 then.

Yes!

Member

orbeckst commented Jun 16, 2017

We should also ditch option 2 then.

Yes!

kain88-de added a commit that referenced this issue Jun 20, 2017

add deprecation warnings for API changes
- closes #1383
- included deprecations:
  - #1373 Timeseries (targeted for 0.17)
    Note that the deprecation for core.Timeseries will always show up;
    this is deliberate so that users WILL see it as it will be gone in
    the next release!
  - #1377 Quick selectors (target 1.0)
  - #782 flags (target 1.0)
- updated CHANGELOG

richardjgowers added a commit that referenced this issue Jun 21, 2017

add deprecation warnings for API changes
- closes #1383
- included deprecations:
  - #1373 Timeseries (targeted for 0.17)
    Note that the deprecation for core.Timeseries will always show up;
    this is deliberate so that users WILL see it as it will be gone in
    the next release!
  - #1377 Quick selectors (target 1.0)
  - #782 flags (target 1.0)
- updated CHANGELOG

@orbeckst orbeckst changed the title from Remove quick selectors to Remove instant selectors Jul 1, 2017

@orbeckst orbeckst added this to To do in release 1.0 Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment