-
Notifications
You must be signed in to change notification settings - Fork 648
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
Comments
My view is that the old "everything is an |
@orbeckst I agree, and I'm working now to make sure that every |
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 Another idea, before I forget: Maybe @tylerjereddy can write a fixer that simply inserts |
I've made a note to look into this, but will probably hold off unless other devs feel that fixers are needed. |
Not exactly related, but I think class |
@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. |
I think another checkpoint might be the |
Never mind... the class instance doesn't even exist at that point. |
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 |
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".
Then when initializing the universe,
|
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 |
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))
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 likeResidue.atoms
,Segment.atoms
,AtomGroup.atoms
gives me a useableAtomGroup
ResidueGroup.masses
andSegmentGroup.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.The text was updated successfully, but these errors were encountered: