Skip to content

Fixed reprs to match 0.15 behaviour#1222

Merged
orbeckst merged 2 commits intodevelopfrom
issue-1221-reprs
Mar 1, 2017
Merged

Fixed reprs to match 0.15 behaviour#1222
orbeckst merged 2 commits intodevelopfrom
issue-1221-reprs

Conversation

@richardjgowers
Copy link
Member

Fixes #1221

Changes made in this Pull Request:

PR Checklist

  • Tests?
  • Docs?
  • CHANGELOG updated?
  • Issue raised/referenced?

@orbeckst
Copy link
Member

See also #1223 (comment) before merging... maybe keep group reprs simpler than before but make sure that list(group) (and array(group)) show the representations of the individual elements.

@richardjgowers
Copy link
Member Author

@orbeckst this restores old behaviour, so can we merge as is? I did think that #1223 might be a good starter issue for someone (who doesn't have mountains of docs still to write)

@jeiros
Copy link
Contributor

jeiros commented Mar 1, 2017

Hi,

I'm happy to finish up this PR. If I understood this correctly, what's missing is defining a __str___ method in the GroupBase class that is smart and only prints the first and last three elements when it's longer than 10?

@richardjgowers
Copy link
Member Author

richardjgowers commented Mar 1, 2017 via email

@jeiros
Copy link
Contributor

jeiros commented Mar 1, 2017

I've got something that is sort of working, implemented in the GroupBase class. It's like this:

def __str__(self):
    name = self.level.name
    if len(self) <= 10:
        return '<{}Group {}>'.format(name.capitalize(), repr(list(self.level)))
    else:
        return '<{}Group {}, ..., {}>'.format(name.capitalize(),
                                              repr(list(self.level)[:3])[:-1],
                                              repr(list(self.level)[-3:])[1:])

My guess is something like this is what you want:

repr(rg)
>>> '<ResidueGroup with 10 residues>'
str(rg)
>>> <ResidueGroup [<Residue MET, 1>,
 <Residue ARG, 2>,
 <Residue ILE, 3>,
...,
 <Residue ALA, 8>,
 <Residue PRO, 9>,
 <Residue GLY, 10>]>

So that the repr from the individual elements that form the group are printed out.
But what would be the equivalent of self.segments or self.residues in the GroupBase class? Or is that not accessible from there, since ResidueGroup, AtomGroup and SegmentGroup inherit from it?

@jeiros
Copy link
Contributor

jeiros commented Mar 1, 2017

Actually, changing self.level to self does that, I think. Since list(self) gives the repr of its elements.

@richardjgowers
Copy link
Member Author

richardjgowers commented Mar 1, 2017 via email

@orbeckst orbeckst self-requested a review March 1, 2017 18:33
@orbeckst orbeckst self-assigned this Mar 1, 2017
Copy link
Member

@orbeckst orbeckst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good – restoring 0.15.0 as far as I can tell; we'll then move forward with #1223 using this as a starting point.

self._ix = ix
self._u = u

def __repr__(self):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are probably going to go back to the terse representation, see #1223 and comments therein.


assert_raises(TypeError, self.ag.center, weights)

def test_representations():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will need similar tests again for #1223.

@orbeckst orbeckst merged commit 23eeda8 into develop Mar 1, 2017
@orbeckst orbeckst deleted the issue-1221-reprs branch March 1, 2017 18:39
@orbeckst
Copy link
Member

orbeckst commented Mar 1, 2017

@jeiros thanks for taking this on. Please continue discussion at #1223 ; once you have a PR referencing #1223 , most of the detailed discussion will happen on the PR.

@jeiros
Copy link
Contributor

jeiros commented Mar 1, 2017

I actually just have finished writing the tests, where should I do the PR so you can start looking at it?

@jeiros
Copy link
Contributor

jeiros commented Mar 1, 2017

Ah sorry, I see. I'll do a new PR referencing #1223

@orbeckst orbeckst mentioned this pull request Mar 1, 2017
4 tasks
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

Successfully merging this pull request may close these issues.

3 participants