Raised meaningful error for short peptide stretch - #5163
Conversation
|
@orbeckst @marinegor please check this, and do I need to add extra tests for this? and also codecov is failing, so I think it would be better if I add tests right? and please tell me what to write in CHANGELOG, cause I'm confused about the format.... :-) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #5163 +/- ##
===========================================
- Coverage 92.72% 92.72% -0.01%
===========================================
Files 180 180
Lines 22472 22474 +2
Branches 3188 3189 +1
===========================================
+ Hits 20838 20839 +1
- Misses 1176 1177 +1
Partials 458 458 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Please add:
- Tests that trigger the error
- Tests that check that the system passes with 6 residues (and that it returns correct results)
- Update the docstring "Raises" section
- Update relevant docs to make it clear that at least 6 residues are necessary
- Update the changelog
Please look at the instructions in the changelog ( Lines 6 to 13 in bbcef1b |
ok |
|
@IAlibay hi! |
|
Hi @marinegor @IAlibay if you are not busy, can you tell me any updates on this? |
|
@spyke7 - I appreciate the ping. I won't get to this until the weekend at the very least. As per all core developers, we tend to not have time during the week days due to life priorities. |
Sure. No problem with that. |
|
@IAlibay I tentatively assigned you here, given that you've been reviewing already. If you don't have the bandwidth please unassign yourself. |
|
Hi @IAlibay |
| ---------- | ||
| atoms : Union[Universe, AtomGroup] | ||
| input Universe or AtomGroup. In both cases, only protein residues will | ||
| input at least 6 Universe or AtomGroup. In both cases, only protein residues will |
There was a problem hiding this comment.
This sentence is incorrect, please fix.
There was a problem hiding this comment.
ok, I need to remove at least 6 right?
There was a problem hiding this comment.
You should mention "6 residues" somewhere, but this current sentence structure doesn't say that.
There was a problem hiding this comment.
input Universe or AtomGroup with at least 6 protein residues. In both cases, only protein residues will
is this good?
There was a problem hiding this comment.
I think so, I'll review when you push the commit.
| resids = protein.residues.resids | ||
|
|
||
| with pytest.raises(ValueError, match="DSSP requires at least 6 residues"): | ||
| res2 = u.select_atoms(f"protein and resid {resids[0]}-{resids[1]}") |
There was a problem hiding this comment.
Please use atomgroup maniuplations here (and below) instead of calling "select_atoms".
| u = mda.Universe(TPR, XTC) | ||
|
|
||
| protein = u.select_atoms("protein") | ||
| resids = protein.residues.resids |
There was a problem hiding this comment.
| resids = protein.residues.resids |
No longer being used?
3 participants
Fixes #5046
Changes made in this Pull Request:
ag.residues, and just raised a simple value error message showing the minimum required residuesPR Checklist
package/CHANGELOGfile updated?package/AUTHORS? (If it is not, add it!)Developers Certificate of Origin
I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.
📚 Documentation preview 📚: https://mdanalysis--5163.org.readthedocs.build/en/5163/