groups.unique does not sort the group

[lilyminium]

Expected behavior

Related to #2977 (comment)

The documentation implies that atomgroup.unique will return a sorted atomgroup, so I expect it to. However, there is a confusing contradiction in the documentation.

    def unique(self):
        """An :class:`AtomGroup` containing sorted and unique
        :class:`Atoms<Atom>` only.

        If the :class:`AtomGroup` is unique, this is the group itself.

Actual behavior

If the atomgroup is already unique, as per docs, the original unsorted atomgroup is returned. @coredevs is this the desired behaviour? Should we change it or add a new sorted attribute?

The .unique attribute is primarily used in select_atoms and wrapping-related methods, i.e. bsphere, wrap, unwrap, translate, and rotate. I don't think being ordered is important in any of those -- but being sorted is probably important to select_atoms? Are any users relying on unique being sorted, or conversely not being sorted?

Code to reproduce the behavior

>>> import MDAnalysis as mda
>>> from MDAnalysis.tests.datafiles import PDB
>>> u = mda.Universe(PDB)
>>> ag = u.atoms[[2, 1, 0]]
>>> ag.unique.ix
array([2, 1, 0])

Current version of MDAnalysis

• Which version are you using? (run python -c "import MDAnalysis as mda; print(mda.__version__)") 2.0.0-dev0
• Which version of Python (python -V)?
• Which operating system?

[orbeckst]

The expectation for selections has been that they come back sorted. Right now that's a defect, at least for 1.x.

[lilyminium]

The expectation for selections has been that they come back sorted.

Would you know the expectation for atomgroup.unique? There are two ways to fix this:

• sort atomgroup.unique and that should fix select_atoms upstream
• add a sorted() method or property that sorts the atomgroup without ensuring uniqueness; alter the selection methods to call atomgroup.sorted().unique. This has the advantage of adding more general functionality to groups too

[ceri-richmond]

Just to say I think you got the wrong @coredevs

[lilyminium]

Oops, sorry! I did mean @MDAnalysis/coredevs.

[orbeckst]

I might have prematurely slapped the "defect" tag on here; it would have been more appropriate on an issue directly referencing select_atoms() because current behavior of unsorted atomgroups clearly breaks stated behavior

Selections always return an AtomGroup with atoms sorted according to their index in the topology (this is to ensure that there are not any duplicates, which can happen with complicated selections).

Regarding your #3364 (comment)

Would you know the expectation for atomgroup.unique?

No, I am not sure. But I don't like the current behavior that will produce erratic results. It would be cleaner if unique just did the same thing as numpy.unique:

Find the unique elements of an array.

Returns the sorted unique elements of an array.

There might have been speed considerations in the current logic somewhere, so I can see that you might not want to sort unless you have to. I can also see that your proposal of decoupling sorting and uniqueness gives more flexibility. IIRC, sorted arrays can be much faster de-duplicated than unsorted ones, though, so that was probably why the two went hand-in-hand. On balance, however, I like re-affirming user expectations, namely that MDAnalysis unique works very similar to the numpy unique, especially as we tend to treat AtomGroups like "numpy arrays of atoms".

[jbarnoud]

At which point of the selection process is unique called? Because if you make a compound selection, then you need to be careful when you sort. Also, selections are sorted because atoms are looked at in order, adding a sort would be unnecessary slow, wouldn't it?

I think the most important point is that if an atomgroup is already not redundant, then calling unique does return the same atomgroup. Sorting it breaks this.

[lilyminium]

At which point of the selection process is unique called?

Somewhat repetitively, at the end of each logical operation selection class (and, or, all, not, global) and at the end of many if not all of the keyword classes. It should probably just be another method on Selection.

Because if you make a compound selection, then you need to be careful when you sort.

Yeah, the select_atoms method sums each selection in compounds so as long as we keep the sorting within the selection classes, order will be retained.

Also, selections are sorted because atoms are looked at in order, adding a sort would be unnecessary slow, wouldn't it?

Yeah, but in the order of the input atom group -- so in the example where the input is u.atoms[[2, 1, 0]], the selection class will inspect the group with the third atom first.

I think the most important point is that if an atomgroup is already not redundant, then calling unique does return the same atomgroup. Sorting it breaks this.

Just to be clear, in your opinion then u.atoms[[2, 1, 0]].unique should return u.atoms[[2, 1, 0]] instead of [0, 1, 2]?

[jbarnoud]

Yeah, but in the order of the input atom group -- so in the example where the input is u.atoms[[2, 1, 0]], the selection class will inspect the group with the third atom first.

This is the behaviours I expect. But I may not be representative. My issue with sorting here, is that I can sort afterward, but I cannot go back to the weird order I may have spent time building.

I think the most important point is that if an atomgroup is already not redundant, then calling unique does return the same atomgroup. Sorting it breaks this.

Just to be clear, in your opinion then u.atoms[[2, 1, 0]].unique should return u.atoms[[2, 1, 0]] instead of [0, 1, 2]?

Indeed.

[lilyminium]

I'm happy to have .unique not sort, but IMO select_atoms is really advertised as sorting the output:

• mdanalysis/package/MDAnalysis/analysis/align.py

  Lines 376 to 380 in 8f9f402

  	.. Warning:: The atom order for `mobile` and `reference` is *only*
  	preserved when `select` is either "all" or ``None``. In any other case,
  	a new selection will be made that will sort the resulting AtomGroup by
  	index and therefore destroy the correspondence between the two groups.
  	**It is safest not to mix ordered AtomGroups with selection strings.**

• mdanalysis/package/MDAnalysis/analysis/align.py

  Lines 478 to 480 in 8f9f402

  	# keep the EXACT order in the input AtomGroups; select_atoms('all')
  	# orders them by index, which can lead to wrong results if the user
  	# has crafted mobile and reference to match atom by atom

• mdanalysis/package/MDAnalysis/analysis/rms.py

  Lines 382 to 383 in 8f9f402

  	:class:`~MDAnalysis.core.groups.AtomGroup` with defined atom order as
  	described under :ref:`ordered-selections-label`).

• mdanalysis/package/MDAnalysis/core/groups.py

  Line 2705 in 8f9f402

  Returns an :class:`AtomGroup` sorted according to their index in the

• https://docs.mdanalysis.org/stable/documentation_pages/selections.html#ordered-selections

We could add a keyword sorted=True?

[lilyminium]

Could we just get a brief consensus please, thumbs up 👍 for sorting and thumbs down 👎 for not sorting groups.unique? (pinging @MDAnalysis/coredevs too)

Pros for sorting

• It follows np.unique
• I think it was previous behaviour until 0.9.2, so
• It is currently documented as sorting

Cons for sorting

• unique != sorted in English. np.unique is not a direct parallel to group.unique; they do not have ndarray.unique.
• If a group is already unique, this will not return the same group but a new one that is sorted
• It is currently documented as returning any group that is unique

(Please feel free to edit this comment with more points)

[IAlibay]

So my understanding here is that if it's not unique it sorts, if it is unique it doesn't sort?

Personally I think that for 2.0 the behaviour should be aligned to the original intended behaviour. Then if we want to re-works this and do an API break then fine, but that should be handled in 3.0.