Fixes sphzone, sphlayer, cyzone, cylayer empty selections

[orionarcher]

Fixes #2915

Changes made in this Pull Request:

• created a new decorator to check for empty sub-expressions in sphzone, sphlayer, cyzone, and cylayer.
• added testing to confirm the fix
• updated CHANGELOG
• updated AUTHORS

PR Checklist

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

[codecov]

Codecov Report

Merging #3202 (b6dfd9a) into develop (3e249fe) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head b6dfd9a differs from pull request most recent head 0cc38ad. Consider uploading reports for the commit 0cc38ad to get more accurate results

@@           Coverage Diff            @@
##           develop    #3202   +/-   ##
========================================
  Coverage    92.73%   92.73%           
========================================
  Files          168      168           
  Lines        22688    22694    +6     
  Branches      3215     3218    +3     
========================================
+ Hits         21040    21046    +6     
  Misses        1600     1600           
  Partials        48       48           

Impacted Files	Coverage Δ
package/MDAnalysis/core/selection.py	98.76% <100.00%> (+0.01%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e249fe...0cc38ad. Read the comment docs.

[orionarcher]

Could someone help me understand what codecov is doing and how I can fix the error it is throwing? EDIT: nvm, it looks like it ended up passing the test!

[lilyminium]

Thanks for looking at this, @orioncohen. LGTM. As this is your first contribution to MDAnalysis, please add yourself to AUTHORS -- please also add your username to the top of CHANGELOG :-)

[lilyminium]

Thanks @orioncohen. Looking more closely, though, the issue actually mentions cyzone and (in a comment lower down) sphlayer. As a more general fix could you look at those too? In fact it may be more elegant to just modify the return_empty_on_apply decorator or make a new decorator, to follow the Don't Repeat Yourself philosophy of Python.

[orionarcher]

Hi @lilyminium. I've added a fix for cyzone, cylayer, and sphlayer too. I tried creating a new decorator to test if the selection is empty. The problem is that we need to check the contents of sel after callingsel = self.sel.apply(group), so the decorator needs to reference an internal method of the object. The best workaround I can think of is to add a decorator method to the Selection base class but that appears to be a meta-class. I ended up going with the naive solution, which required adding 4 more lines of code to fix cyzone, cylayer, and sphlayer. Thanks for the help!

[lilyminium]

so the decorator needs to reference an internal method of the object. The best workaround I can think of is to add a decorator method to the Selection base class but that appears to be a meta-class.

Your solution does fix the problem, but I'm surprised you can't just decorate the apply() methods of the relevant Selection classes the same way that return_empty_on_apply has been used.

[pep8speaks]

Hello @orioncohen! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-05 19:48:03 UTC

[orionarcher]

I should have taken a closer look when I was less sleepy! I implemented this with a decorator as you suggested. I added a new decorator rather than modifying return_empty_on_apply because not all of the selection methods call self.sel.apply(group). I also moved around my testing to be a bit more consistent.

[lilyminium]

Thanks @orioncohen, the decorator is much nicer than before! I just have some design comments. @richardjgowers you know more than I do about selections, so please weigh in if I've said anything dumb.

[lilyminium]

Other than the CHANGELOG message, LGTM!