Paml parsetests #29

Closed
wants to merge 28 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

brandoninvergo commented Feb 22, 2012

I updated all of the unit tests for the parsing of output by the PAML programs. A few changes were made to the Bio.Phylo.PAML module to fix failures with the new tests. In addition, I cleaned up the Tests/PAML resource directory to be cleaner and more structured. Finally, I added a script to that directory to provide a way to automatically generate the results files which are used in the tests, making future maintenance of the tests a bit more viable.

brandoninvergo added some commits Feb 2, 2012

Owner

peterjc commented Feb 22, 2012

Great - I hope to look at this this afternoon.

Owner

peterjc commented Feb 22, 2012

Hi Brandon,

You can't use the unittest assertIn(a, b, msg) method yet since it requires Python 2.7+, so assertTrue(a in b, msg) would be the best alternative.

Also a "git rebase master" would be nice - there seemed to be one minor merge conflict with 2876d12#Bio/Phylo/PAML/_parse_codeml.py where I assume the desired change is to:

if ":" in line or "'#" in line:

Peter

Contributor

brandoninvergo commented Feb 23, 2012

Hi Peter,

I'll change the assertIns to assertTrues today as recommended.

Regarding the merge conflict, I did a git merge master, but git replied that everything was up-to-date. Before that I had done a git merge upstream master but I forgot to checkout the master branch first, so upstream was merged into this branch (see commit f68co4b), which happened without any conflicts (and after recognizing my error, merged upstream into master as well).

It should be if ":" in line or "#" in line: (no apostrophe before the hash).

So, I'm not sure what I did wrong to cause this conflict. I'll double-check my local codebase.

Contributor

brandoninvergo commented Feb 23, 2012

Upon further inspection, both my master branch and this paml-parsetests branch, as well as the official Biopython master branch all have the same thing on that line: if ":" in line or "#" in line:

What the heck did I do wrong?

Owner

peterjc commented Feb 23, 2012

I'm guessing your branch began before the if ":" in line or "#" in line: fix, and you fixed it both on the master and on the branch, and that may be why 'git rebase master' triggered a merge.

The 'git rebase master' isn't essential - it just means the final history on the master would look like one long branch (with some old commits checked in recently), versus two parallel branches with several merges. I find the one branch version much easier conceptually, and also it makes bisecting any regressions easier.

I'll keep an eye out for an update regarding the asserts. Thanks

Contributor

brandoninvergo commented Feb 23, 2012

Hm ok, I'll keep that in mind. I'm familiar with git rebase but I've heard that it complicates things for collaborative work. Anyway, this is the only project for which I collaboratively use git, and given the time between my pull requests, it becomes a slow learning process full of bumps in the road. Thanks for your patience as always!

Anyway, I just pushed the assertIn->assertTrue changes to this branch and I confirmed that the tests still pass.

Owner

peterjc commented Feb 23, 2012

Another issue, sorry - just tried in on Jython but this would also break on Python 2.5

ERROR: testParseAlpha1Rho1 (test_PAML_baseml.ModTest)

Traceback (most recent call last):
File "test_PAML_baseml.py", line 242, in testParseAlpha1Rho1
version_msg = "Improper parsing for model {0} version {1}".format(
AttributeError: 'str' object has no attribute 'format'

The string object's format method was added in Python 2.6, and we still want to support Python 2.5 (and Jython).

I'm working on it now...

Contributor

brandoninvergo commented Feb 23, 2012

Ah, shoot, I knew that...it's a force of habit. I'll fix it now.

Owner

peterjc commented Feb 23, 2012

I've fixed it locally. Testing now...

Contributor

brandoninvergo commented Feb 23, 2012

Fixed and pushed

Contributor

brandoninvergo commented Feb 23, 2012

Oops, I just got your new message....nevermind my push.

Owner

peterjc commented Feb 23, 2012

Not to worry - the changes should now be on the master. I'm about to start the buildslaves, but that should be good to go.
f9866be

Do you want to add anything to the NEWS file?

Peter

Contributor

brandoninvergo commented Feb 23, 2012

Aside from the unit tests, there were just some small bug fixes: I added the previously omitted "rho" and "fix_rho" parameters for codeml, and I implemented various small parsing fixes, mostly to handle the output of particular PAML versions.

I'm not sure if those are notable enough for anything more than a cursory "Bio.Phylo.PAML: various bug fixes" note.

Owner

peterjc commented Feb 23, 2012

OK, thanks!

@peterjc peterjc closed this Feb 23, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment