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

Quick fix for atommethods to return empty residue group #3089

Merged
merged 8 commits into from
Jan 4, 2021

Conversation

aditya-kamath
Copy link
Contributor

@aditya-kamath aditya-kamath commented Jan 1, 2021

Partially fixes #2879

Changes made in this Pull Request:

  • Returns empty residue group for _get_prev_residues_by_resid and _get_next_residues_by_resid

PR Checklist

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

Returns empty residue group for _get_prev_residues_by_resid and _get_next_residues_by_resid
@pep8speaks
Copy link

pep8speaks commented Jan 1, 2021

Hello @aditya-kamath! 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-01-04 03:06:30 UTC

@IAlibay
Copy link
Member

IAlibay commented Jan 1, 2021

Thanks for this PR @aditya-kamath. Not a review (I'll probably let @lilyminium handle this as she has a much better understanding of the issue), but just some passing quick comments:

  1. Please remember to add yourself to AUTHORS and add an entry to CHANGELOG
  2. Since this is fixing a very specific failure, you should aim to add some tests for the edge case that this is attempting to fix.
  3. In your PR message, could you add the issue number this is partially fixing so that reviewers know what this is trying to fix?
  4. Please have a look & fix the PEP8speaks issue.

@codecov
Copy link

codecov bot commented Jan 1, 2021

Codecov Report

Merging #3089 (14acdfb) into develop (8f7e338) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop    #3089   +/-   ##
========================================
  Coverage    93.17%   93.17%           
========================================
  Files          171      171           
  Lines        22729    22735    +6     
  Branches      3216     3216           
========================================
+ Hits         21177    21183    +6     
  Misses        1504     1504           
  Partials        48       48           
Impacted Files Coverage Δ
package/MDAnalysis/core/topologyattrs.py 96.68% <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 8f7e338...14acdfb. Read the comment docs.

@aditya-kamath aditya-kamath changed the title Quick fix for atommethods to return emprty residue group Quick fix for atommethods to return empty residue group Jan 1, 2021
@aditya-kamath
Copy link
Contributor Author

Okay, Thanks @IAlibay! Happy to put up my first PR.
I assume by adding tests you mean that I write the code which reproduces this behavior. (?)
I'll patch up the other tasks.

@IAlibay
Copy link
Member

IAlibay commented Jan 1, 2021

I assume by adding tests you mean that I write the code which reproduces this behavior. (?)

Yes, we use pytest for code testing in MDAnalysis. Under mdanalysis/testsuite/MDAnalysis/core you'll find a file called test_topologyattrs.py.

Within it you'll need to add one or more test functions that cover the edge case (i.e. ensuring that you get an empty residue group return) that is being fixed here.

If you aren't used to pytest (please ignore this if that's not the case); I would suggest doing the tests for the two functions you have fixed now before moving on to fixing the rest. This way it's easier to work through the code with you. Apologies for the brief explanation of what's needed test-wise, I don't want to presume your current knowledge of pytest. Do let us know if you want a more detailed walkthrough of it all.

@IAlibay
Copy link
Member

IAlibay commented Jan 1, 2021

@aditya-kamath any reason why this was closed?

@aditya-kamath
Copy link
Contributor Author

Hi, @IAlibay. I am a complete beginner with this. So, your explanation on pytest is helpful! :)
I figure I should close this and reopen it when I have done the things you've asked for?

@lilyminium lilyminium reopened this Jan 1, 2021
@lilyminium
Copy link
Member

No need, @aditya-kamath, it's common to go back and forth on a pull request :-) Let's just leave it open for now -- it also allows tests to run as a check on code.

Changed authors, changelog to add my name
Tests topologyattrs included
Copy link
Member

@lilyminium lilyminium left a comment

Choose a reason for hiding this comment

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

Thanks for adding a test, @aditya-kamath! Just a few comments.

testsuite/MDAnalysisTests/core/test_topologyattrs.py Outdated Show resolved Hide resolved
package/AUTHORS Show resolved Hide resolved
Changed the test to make it a function instead of a class
@aditya-kamath
Copy link
Contributor Author

Hi @IAlibay @lilyminium, it looks like the checks have passed! Thanks for your feedback and comments :D

@lilyminium
Copy link
Member

It's looking good, thank you @aditya-kamath! Could you just address the pep8 comments in #3089 (comment)? Then you're done :-)

@lilyminium lilyminium merged commit 87e71bb into MDAnalysis:develop Jan 4, 2021
@lilyminium
Copy link
Member

Great, thank you @aditya-kamath! Congratulations on your first commit to MDAnalysis! 🎉

cbouy pushed a commit to cbouy/mdanalysis that referenced this pull request Jan 12, 2021
* Add arbitrary TopologyAttr selection (MDAnalysis#2927)

Fixes MDAnalysis#2925 
Fixes MDAnalysis#2875
Fixes MDAnalysis#3054 

Changes made in this Pull Request:
 - added a class factory to subclass `core.selection.Selection` for each TopologyAttr
 - added tokens to `core.selection.SameSelection`
 - added `FloatRangeSelection` and `BoolSelection`
 - added negatives, scientific notation and "to" delimiter for ranges

* Add ReadTheDocs configuration for PR builds (MDAnalysis#3060)

 - Adds RTD configuration
 - Add `environment.yml` for package installation

* Remove appveyor

* Install MDAnalysis on ReadTheDocs via pip (MDAnalysis#3071)

Install via `pip install package/` to build current docs on ReadTheDocs

* try stringio

* rm metals file

* pin pytest

* pin pytest on gh actions

* Fixes RMSF docstring (Issue MDAnalysis#2806) (MDAnalysis#3033)

Fixes the RMSF docstring's align command and adds transformation to make the results accurate

* MAINT: simplify guessers regex (MDAnalysis#3085)

* the `SYMBOLS` regex in `guessers.py` does not require
any escape sequences because the metacharacters are inactive
in the character class (this includes the range metacharacter
when placed at the start or end of the character class)

* MAINT: char class regex improve

* avoid the overhead of a regex character class
when that character class has only a single character
(i.e., serves no purpose)

* there is only one instance of this in MDA codebase
discovered by my [scraping
code](https://github.com/tylerjereddy/regex-improve)

* for a longer explanation see my similar changes in
NumPy codebase:
numpy/numpy#18083

* Fix syntax warning over comparison of literals using is.

* Quick fix for atommethods to return empty residue group (MDAnalysis#3089)

Returns empty residue group for _get_prev_residues_by_resid and _get_next_residues_by_resid

* Add to authors list.

Co-authored-by: Lily Wang <[email protected]>
Co-authored-by: IAlibay <[email protected]>
Co-authored-by: Tyler Reddy <[email protected]>
Co-authored-by: Lily Wang <[email protected]>
Co-authored-by: Irfan Alibay <[email protected]>
Co-authored-by: Oliver Beckstein <[email protected]>
Co-authored-by: Karthikeyan Singaravelan <[email protected]>
Co-authored-by: Aditya Kamath <[email protected]>
PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021
)

Returns empty residue group for _get_prev_residues_by_resid and _get_next_residues_by_resid
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Atomnames methods should handle empty group
5 participants