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

topology API semantics test case #703

Closed
2 tasks
orbeckst opened this issue Feb 8, 2016 · 11 comments
Closed
2 tasks

topology API semantics test case #703

orbeckst opened this issue Feb 8, 2016 · 11 comments

Comments

@orbeckst
Copy link
Member

orbeckst commented Feb 8, 2016

Just had a discussion with @dotsdl in the context of PR #698 : Before merging the PR for issue #363 we should have a test_topology_semantics.py that tests how we expect the API to behave. This would include things like

  • Residue.atoms , Segment.atoms, AtomGroup.atoms gives me a useable AtomGroup
  • The aggregated attributes such as ResidueGroup.masses and SegmentGroup.masses behave as expected.

... and more — Please add!

We are already testing a lot of this stuff in the test_atomgroup.py tests but I'd like to focus here on the semantics and how we present the topology information to users. Furthermore, adding tests that test for new desired behavior and fail currently are also encouraged. Ultimately, we will need a high level overview over the philosophy and semantics because convenient and intuitive access to groups of atoms is probably one of our biggest strengths at the moment.

@orbeckst orbeckst added the API label Feb 8, 2016
@orbeckst orbeckst added this to the Topology refactor - 0.15 milestone Feb 8, 2016
@orbeckst
Copy link
Member Author

orbeckst commented Feb 8, 2016

My view is that the old "everything is an AtomGroup" was good from the user's point of view because you don't need to know what object you actually have (I really dislike needing to ask what kind of instance I hold before I know what I can do with the object). I see having separate ResidueGroup, Residue, SegmentGroup, Segment more as an internal book-keeping device that allows us to prevent staleness. I'd like to be able to still treat these different groups (mostly) as old AtomGroups in the sense of duck-typing: any attribute or method that makes sense for the group should be there (such as centroid()).

@dotsdl
Copy link
Member

dotsdl commented Feb 8, 2016

I'd like to be able to still treat these different groups (mostly) as old AtomGroups in the sense of duck-typing: any attribute or method that makes sense for the group should be there (such as centroid()).

@orbeckst I agree, and I'm working now to make sure that every TopologyAttr method that's being added to AtomGroup also makes it to Residue, ResidueGroup, Segment, and SegmentGroup if appropriate. However, it's always possible to get an AtomGroup from any of these objects with .atoms. That's not sufficient, though?

@orbeckst
Copy link
Member Author

orbeckst commented Mar 1, 2016

You're right (and I might have been flip-flopping or to-froing or up-downing on this one for a while). Being able to go through .atoms is crucial and should become the new idiom. One other issue had a long discussion on all of this (maybe #599 ?) but I don't have the time for a summary.

Another idea, before I forget: Maybe @tylerjereddy can write a fixer that simply inserts .atoms before any of the old methods of AtomGroup to soften the transition? Would this be worthwhile, i.e., have people been using ten2eleven? (I have...)

@tylerjereddy
Copy link
Member

I've made a note to look into this, but will probably hold off unless other devs feel that fixers are needed.

@khuston
Copy link
Contributor

khuston commented Mar 21, 2016

Not exactly related, but I think class Topology or TransTable should raise an Exception if parameters residue_segindex or atom_resindex are not the expected lengths (n_residues and n_atoms, respectively). Right now, if someone specifies an array that is longer, it will quietly truncate the array. Probably not many users will be using these directly, but this can catch problems with topology readers.

@dotsdl
Copy link
Member

dotsdl commented Mar 21, 2016

@khuston agreed. There probably many cases where the new topology system does not have internal robustness checks, so there's plenty of room for work here.

@khuston
Copy link
Contributor

khuston commented Mar 21, 2016

I think another checkpoint might be the GroupBase._add_prop method, making sure that len(attr) == len(self).

@khuston
Copy link
Contributor

khuston commented Mar 21, 2016

Never mind... the class instance doesn't even exist at that point.

@richardjgowers
Copy link
Member

The len of an attr is a little confusing to define, for instance with bonds there'll be less bonds than atoms. All an attr has to do is respond to a request from every atom.

If you want to add some exceptions and tests feel free, we haven't made everything bulletproof yet

@khuston
Copy link
Contributor

khuston commented Mar 21, 2016

Ah yeah, I was thinking the superclass (AtomAttr, ResidueAttr, etc.) corresponded exactly with the length of the attribute value, but that's not the case. How would you feel about adding an attribute of toplogyattrs "per_object" that can be set to "atom", "residue" or "segment".

class Radii(AtomAttr):
    """Radii for each atom"""
    attrname = 'radii'
    singular = 'radius'
    per_object = 'atom'

class Resids(ResidueAttr):
    """Residue ID"""
    attrname = 'resids'
    singular = 'resid'
    target_levels = ['atom', 'residue']
    per_object = 'residue'

Then when initializing the universe,

    n_dict = {'atom': self._topology.n_atoms,
              'residue': self._topology.n_residues,
              'segment': self._topology.n_segments}
    # Put Group level stuff from topology into class
    for attr in self._topology.attrs:
        if hasattr(attr, 'per_object') and \
                len(attr) != n_dict[attr.per_object]:
            raise ValueError('Length of {attr} does not'
                             ' match number of {obj}s.\n'
                             'Expect: {n:d} Have: {m:d}'.format(
                                 attr=attr.attrname,
                                 obj=attr.per_object,
                                 n=n_dict[attr.per_object],
                                 m=len(attr)))
        self._process_attr(attr)

@richardjgowers
Copy link
Member

So we can optionally declare that some attrs must be the same length to make things safer? Definitely can't hurt.

I'd only want the check to happen once, so I think the place you've put it now is universe init.. but we'd also want that check when adding Attrs post creation, so maybe move it to _process_attr

dotsdl added a commit that referenced this issue Mar 28, 2016
We want ResidueGroups and SegmentGroups to behave roughly in the same
way as before, that is having methods they had when they were subclasses
of AtomGroup. Not everything makes sense, but most things do.

This was consensus from discussion in #703
(#703 (comment))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants