PyRosetta demos of working RNA apps

[everyday847]

Do review; I hardly ever write code with pyrosetta.

[lyskov]

@everyday847 i will look at the code and let you know. But in the mean while: have you considered adding tests for your app? It should be really straightforward thing to do, for example of how to do this please see: #3

[lyskov]

Andy, code looks good to me! A few general things:

• have you tested this with both Python-2 and Python-3? It does not look like you have anything Python-2 specific there so it maybe already ok.

• is this a library scripts or an app scripts? I mean if it could be used already as-is (seems to me so) then my question is: are you sure about the naming for this apps? I mean there is 'clear' and 'demo' in it. (i am not saying that you should not name it like that, - maybe that was your intention all along! I am saying that if such naming is leftover product of old intention then maybe this could be refactored to something that is less confusing for users?)

[lyskov]

Also, we can start running tests for this as soon as issue for mounting point for pyrosett_scripts repository is resolved (see my email to devel list)

[everyday847]

Sure -- I have test cases I've been running on, anyway, so I'll add them. I've only tested with python2 because I didn't want a separate PyRosetta compile. Is there a way to get around that?

There's probably no harm in changing the names to just stepwise and rna_denovo, so I'll do that.

[everyday847]

#3 seems like a good example of adding unit tests, but I don't have anything nontrivially unit-testable -- I'd rather write a regression test, which seems like what the point of the tests/ directory will be.

I guess I could have each of my main app functions return the pose and write wrappers that dump it to PDB, and then test that PDB string against what it is currently. But that seems pretty ugly.

[lyskov]

Andy, how about functional test then? Ie something that will just run the script to make sure code could be imported and at least some of the if/for branches is tested. One of the biggest problem with Python code (and big difference from C++) is that unless we run code routinely it is trivial to break it without noticing it. For example misspelling variable name will be a hard error which such test will be able to catch. And functional test will be the exactly right amount thing for this. (it looks to me that you probably does not care if code starting to produce different output because almost all machinery is in C++ and not in Python, so creating an integration test for this seems wrong)

[lyskov]

Re integration tests in general: right now we do not support it but we might do that in the future (i will try to avoid adding this thought)

Also, I merged #3 and corresponding PR in master and tests for this revision now will be run routinely for each new commit in both master and commits branches.

[everyday847]

Right, I'm totally into writing a functional test or two. Let me set that up tonight.

…

On Mon, Sep 11, 2017 at 2:35 PM, Sergey Lyskov ***@***.***> wrote: Andy, how about functional test then? Ie something that will just run the script to make sure code could be imported and at least some of the if/for branches is tests. One of the biggest problem with Python code (and big difference from C++) is that unless we run code routinely it is trivial to break it without noticing it. For example misspelling variable name will be a hard error which such test will be able to catch. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AExsa1CPiZ91I8iNXMDBQKkvMyEKPmQ5ks5shae1gaJpZM4PQXxj> .

[lyskov]

Great! I think all scripts and machinery is already in place for this. So when you added your test just update HEAD of submodule in main and it should trigger the run.

[everyday847]

I'd just drop the test(s) in PyRosetta/src/test? (There's an empty test directory in pyrosetta_scripts allegedly for integration tests; I'm guessing we don't have that mechanism.

…

On Mon, Sep 11, 2017 at 2:59 PM, Sergey Lyskov ***@***.***> wrote: Great! I think all scripts and machinery is already in place for this. So when you added your test just update HEAD of submodule in main and it should trigger the run. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AExsa1UNwDr1ZH1RDvW0A9jjVX6a7-43ks5sha1HgaJpZM4PQXxj> .

[lyskov]

(There's an empty test directory in pyrosetta_scripts allegedly for integration tests; I'm guessing we don't have that mechanism.

correct!

I'd just drop the test(s) in PyRosetta/src/test?

no, - you will need to create test_<app name>.py in this repository (i suggest putting it in the same dir as your app scripts. Please see example organization here: https://github.com/RosettaCommons/pyrosetta_scripts/tree/master/apps/%2Bexample

[lyskov]

So in your case dir will be: https://github.com/RosettaCommons/pyrosetta_scripts/tree/5657d842dc99caaba79668a6404c8f1ade16f3bc/apps/rna/awatkins

Also, are you sure about the names of your apps? (if it do something useful maybe _demo is not needed and will be confusing for users?)

[everyday847]

Yeah, I changed their names but hadn't psuhed in a while.

…

On Mon, Sep 11, 2017 at 5:37 PM, Sergey Lyskov ***@***.***> wrote: So in your case dir will be: https://github.com/RosettaCommons/pyrosetta_ scripts/tree/5657d842dc99caaba79668a6404c8f1ade16f3bc/apps/rna/awatkins Also, are you sure about the names of your apps? (if it do something useful maybe _demo is not needed and will be confusing for users?) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AExsa57xFMj4fP31xIhrTnbt0g2Sz-QZks5shdJRgaJpZM4PQXxj> .

[everyday847]

So I can put test input PDB files into this repo somewhere? I can't run either of these functions on poses generated just from sequence, and I need a fasta input file to boot. On Mon, Sep 11, 2017 at 5:39 PM, Andy Watkins <andy.watkins2@gmail.com> wrote:

…

Yeah, I changed their names but hadn't psuhed in a while. On Mon, Sep 11, 2017 at 5:37 PM, Sergey Lyskov ***@***.***> wrote: > So in your case dir will be: https://github.com/RosettaComm > ons/pyrosetta_scripts/tree/5657d842dc99caaba79668a6404c8f1ad > e16f3bc/apps/rna/awatkins > > Also, are you sure about the names of your apps? (if it do something > useful maybe _demo is not needed and will be confusing for users?) > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#4 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AExsa57xFMj4fP31xIhrTnbt0g2Sz-QZks5shdJRgaJpZM4PQXxj> > . >

[everyday847]

How does this look?

[lyskov]

Andy, this looks great! Small note:
the assert True is left over from unit test real functionality (ie originally it should be assert a==b or something) so i think it could be removed from tests functions bodies