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

make empty sphzone / cyzone selections consistent with 'around' #2915

Closed
xiki-tempula opened this issue Aug 19, 2020 · 7 comments · Fixed by #3202
Closed

make empty sphzone / cyzone selections consistent with 'around' #2915

xiki-tempula opened this issue Aug 19, 2020 · 7 comments · Fixed by #3202

Comments

@xiki-tempula
Copy link
Contributor

xiki-tempula commented Aug 19, 2020

Is your feature request related to a problem?

Since sphzone will do a center of geometry calculation first, if an empty atomgorup is passed to the sphzone, a not very useful error message will be given.

Traceback (most recent call last):
    water = u.select_atoms( 'resname SOL and sphzone 3 (resid 12 and name HE)')
  File "/Users/grte2001/GitHub/mdanalysis/package/MDAnalysis/core/universe.py", line 677, in select_atoms
    return self.atoms.select_atoms(*args, **kwargs)
  File "/Users/grte2001/GitHub/mdanalysis/package/MDAnalysis/core/groups.py", line 2863, in select_atoms
    selections[0].apply(self))
  File "/Users/grte2001/GitHub/mdanalysis/package/MDAnalysis/core/selection.py", line 136, in apply
    rsel = self.rsel.apply(group)
  File "/Users/grte2001/GitHub/mdanalysis/package/MDAnalysis/core/selection.py", line 168, in apply
    return func(self, group)
  File "/Users/grte2001/GitHub/mdanalysis/package/MDAnalysis/core/selection.py", line 333, in apply
    ref = sel.center_of_geometry().reshape(1, 3).astype(np.float32)
ValueError: cannot reshape array of size 0 into shape (1,3)

Describe the solution you'd like

An error message which says the selection is empty could be given.

EDIT: Following the discussion below and on #3026 we want these selections to behave like around, i.e., just return an empty AtomGroup. — @orbeckst

Describe alternatives you've considered

Just return an empty atomgroup.

@IAlibay
Copy link
Member

IAlibay commented Aug 19, 2020

That's odd, I can't seem to reproduce this locally with:

u = mda.Universe(GRO, XTC)
u.select_atoms('resname SOL and sphzone 3 (resid 12 and name HA)')

@xiki-tempula xiki-tempula changed the title sphzone should allow the the selection of a single atom Give more useful error message when empty atomgroup is given to sphzone Aug 19, 2020
@xiki-tempula
Copy link
Contributor Author

@IAlibay Sorry, I noticed that I have made an error and this is a different problem.

@IAlibay
Copy link
Member

IAlibay commented Aug 19, 2020

So the behaviour here is inconsistent between around & sphzone/cyzone. In the former if the selection is an empty atomgroup we just return an empty atomgroup:

# All atoms in group that aren't in sel
sys = group[~np.in1d(group.indices, sel.indices)]
if not sys or not sel:
return sys[[]]

whilst in the latter selections that check isn't being made.

My proposal here would be to align everything to around. IMHO it's a more commonly used selection keyword and the behaviour would more likely be the one that would be expected?

@orbeckst
Copy link
Member

I agree with #2915 (comment) - make sphzone/cyzone behave in the same way as around and return and empty AtomGroup.

@orbeckst
Copy link
Member

Note that some more details are shown in the duplicate issue #3026.

@orbeckst orbeckst changed the title Give more useful error message when empty atomgroup is given to sphzone make empty sphzone / cyzone selections consisent with 'around' Dec 31, 2020
@orbeckst orbeckst changed the title make empty sphzone / cyzone selections consisent with 'around' make empty sphzone / cyzone selections consistent with 'around' Dec 31, 2020
@sulays
Copy link
Contributor

sulays commented Mar 12, 2021

Hi, I was going through the code and found out that many other selections are also not consistent with around and they don't return an empty AtomGroup (e.g sphlayer). Should I create a PR for changes in all such selections?

@xiki-tempula
Copy link
Contributor Author

Hi @sulays Thanks for your interest. I think this is a very good idea. It might be good to raise these selections as issues (explain what you expect and what you got) and then make a PR against them.

orionarcher added a commit to orionarcher/mdanalysis that referenced this issue Apr 3, 2021
…. Sphzone operating on an empty selection now returns an empty atom group.
orionarcher added a commit to orionarcher/mdanalysis that referenced this issue Apr 3, 2021
orionarcher added a commit to orionarcher/mdanalysis that referenced this issue Apr 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants