Closed gregmedlock closed 5 years ago
Related to this and my previous PR (#586), it looks like annotation/SBO could use some additional tests. I'm happy to help brainstorm a bit of what is needed and implement them as I continue to work through some of these edge cases.
Merging #587 into develop will increase coverage by
0.15%
. The diff coverage is100%
.
@@ Coverage Diff @@
## develop #587 +/- ##
==========================================
+ Coverage 79.95% 80.1% +0.15%
==========================================
Files 57 57
Lines 2929 2931 +2
Branches 388 390 +2
==========================================
+ Hits 2342 2348 +6
+ Misses 521 518 -3
+ Partials 66 65 -1
Impacted Files | Coverage Δ | |
---|---|---|
memote/support/annotation.py | 100% <ø> (+2.5%) |
:arrow_up: |
memote/support/sbo.py | 100% <100%> (ø) |
:arrow_up: |
memote/suite/tests/test_annotation.py | 100% <0%> (+3.06%) |
:arrow_up: |
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 807bfab...b3c5a18. Read the comment docs.
Hi Greg! What were you thinking of? In general, I'm all ears for ideas on additional tests, but I'd also like to point to some existing issues that need some love:
SBO:
Annotation:
re: the issue with test_transport_reaction_specific_sbo_presence
, it looks like that specific case is covered if any of the SBO transport terms are in elem.annotation['sbo']
. I've updated the PR to check whether the terms are a list and handle accordingly. Seems a bit brute force but it gets the job done.
If more complex checks arise, e.g. check that multiple SBO terms are all in an element's annotation, I think another function is warranted (e.g. support.sbo.check_component_for_multiple_sbo_terms
).
I'll add tests for the changes in this PR and continue the higher level discussion in the other issues (#469 should probably come first).
In my second commit I accidentally slipped in an update to fix the gene annotation compliance check. If OK, I'll keep it in this PR.
One more clarification... I was planning on adding tests for the gene annotation compliance checks, but wanted to make sure I wasn't missing them somewhere. My intuition is that they should be located in test_for_annotation.py
but just haven't been implemented yet. If this is correct, please let me know and I'll add the gene-equivalent of the existing metabolite/reaction tests within that file first thing tomorrow. Thanks!
One more clarification... I was planning on adding tests for the gene annotation compliance checks, but wanted to make sure I wasn't missing them somewhere. My intuition is that they should be located in test_for_annotation.py but just haven't been implemented yet.
memote tests
, or memote checks
if you prefer, go in memote.support
,memote tests
go in memote.suite.tests
,unit tests
for the support functions go in tests.test_for_support
.If I understand you correct you are talking about unit tests
and in that case you are right to put them in test_for_annotation
.
I think we're good to go with the latest commit!
CI fails due to pandas changes (see #593). I'm merging anyway and pinning pandas to a lower version for now.
Another small change in annotation checking--the previous line would lead to missing SBO terms for any object with more than one SBO term (e.g. it would end up trying things like
['SBO:0000176', 'SBO:0000185'] in ['SBO:0000185']
).