Agreed about not constructing what's already available and costly to construct! Speaking of consistency, 'selection' of 'atom_selection_cache' uses 'string' as the argument, not selstr as in 'model', example: mmtbx/utils/tst_switch_rotamers.py:   sel = ph.atom_selection_cache().selection(string = "not resname TYR") I see the different between these flavors of select*. Ok, I afraid I'll have to read the code each time I use these.. Pavel On 8/16/18 16:01, Oleg Sobolev wrote:

Hi Pavel,

Currently model class has three selection methods:

def selection(self, selstr, optional=True):

Takes selection string, returns bool selection

def select(self, selection):

Takes selection (bool or iselection), applies it to self and returns selected part of model object.

def iselection(self, selstr):

Same as first, but returns iselection.

which is hideously confusing with zero chance to remember, and also inconsistent with the rest.

Let me disagree on the point of inconsistency with the rest. selection and iselection functions are available with exactly same meaning for atom_selection_cache. select is available with exact same meaning (returning part of an object) in hierarchy, af_shared_atom, flex.vec3_double, xray_structure etc which you pointed out.

Can we consolidate all three into one ".select()" that would allow taking bool or int or str?

No, because they are doing different things. Selection and iselection wraps atom_selection_cache functionality and not duplicating those of select() function. ".select()" function already has the described functionality.

Or you think there are philosophical arguments against it?

Yes, and practical ones as well. I'm caching atom_selection_cache inside model object, so you don't have to construct it again if you use wrappers available in model class. hierarchy.atom_selection_cache() is not a free function to the point I had to pass the cache around to avoid constructing it again to achieve measurable speedup. Especially for larger models.

At the very least, please use model.get_atom_selection_cache() instead of constructing it.

Best regards, Oleg Sobolev.

Pavel

On 8/16/18 15:00, CCTBX commit wrote:

...

Repository : ssh://g18-sc-serv-04.diamond.ac.uk/cctbx On branch  : master

------------------------------------------------------------------------

commit c98b44997e8eb7bf53ee828f510b933cd5167758 Author: Oleg Sobolev mailto:[email protected] Date:   Thu Aug 16 15:00:20 2018 -0700

    Proper use of model class

------------------------------------------------------------------------

c98b44997e8eb7bf53ee828f510b933cd5167758 mmtbx/model/model.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/mmtbx/model/model.py b/mmtbx/model/model.py index 6f2247452..87412f95c 100644 --- a/mmtbx/model/model.py +++ b/mmtbx/model/model.py @@ -2312,8 +2312,7 @@ class manager(object):      sizes = flex.int http://flex.int()      h = self.get_hierarchy()      if(macro_molecule_only): -      asc = h.atom_selection_cache() -      s = asc.selection("protein or nucleotide") +      s = self.selection("protein or nucleotide")        h = h.select(s)      for r in h.residue_groups():        sizes.append(r.atoms().size())

_______________________________________________ cctbxbb mailing list [email protected] mailto:[email protected] http://phenix-online.org/mailman/listinfo/cctbxbb http://phenix-online.org/mailman/listinfo/cctbxbb