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

compiling from py2 to py3; enable tcoffee aligner, iqtree #442

Merged
merged 10 commits into from
Mar 28, 2020

Conversation

dengzq1234
Copy link
Contributor

python 3.x compilation:
- remove deprecated 'U' mode
- raw_input() -> input()
- print()
ete_build tools:
- enable experimental tcoffee aligner
- fix bug of iqtree

VERSION Outdated
3.1.18.1
Copy link
Member

Choose a reason for hiding this comment

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

could we stick to 3 digits versioning?
I tried to used the following convention: if version is A.B.C, A=Major changes (w/ potential compatibility problems). B=Add on features. C=Bug fixes and small changes

@@ -126,7 +126,8 @@ def __init__(self, newick=None, alignment=None, alg_format="fasta",
self.workdir = '/tmp/ete3-tmp/'
if not binpath:
ete3_path = which("ete3")
binpath = os.path.join(os.path.split(ete3_path)[0], "ete3_apps", "bin")
#binpath = os.path.join(os.path.split(ete3_path)[0], "ete3_apps", "bin") #Ziqi
Copy link
Member

Choose a reason for hiding this comment

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

remove commented line


if ete3_path:
builtin_apps_path = os.path.join(os.path.split(ete3_path)[0], "ete3_apps/bin") #Ziqi
#builtin_apps_path = os.path.split(ete3_path)[0]
Copy link
Member

Choose a reason for hiding this comment

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

clean up commented code

@@ -135,7 +136,8 @@ def get_call(appname, apps_path, exec_path, cores):
except KeyError:
return None

bin_path = os.path.join(apps_path, "bin")
#bin_path = os.path.join(apps_path, "bin")
Copy link
Member

Choose a reason for hiding this comment

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

remove comment

valid_cogs = sorted([sing for sing in all_singletons if len(sing) >= min_species],
sort_cogs_by_size)
sort_cogs_by_size) #Ziqi issue
Copy link
Member

Choose a reason for hiding this comment

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

I remember this line was linked to a bug in Python 3. Is it solved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's fixed now

Copy link
Member

@jhcepas jhcepas left a comment

Choose a reason for hiding this comment

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

All good Ziqi. just minor comments for clean up

raise ConfigError("IQTREE CODON models require a codon alignmen.\nProvide nucleotide sequences with '-n' and set '--nt-switch-thr 0.0' to ensure codon alignments.")
#print(GLOBALS["seqtypes"])
#print(seqtype)
#if seqtype == "aa" or "aa" not in GLOBALS["seqtypes"]: #logic confusion Ziqi
Copy link
Member

Choose a reason for hiding this comment

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

could you explain what was the issue here? did you check that your version works as expected?

@@ -16,7 +16,7 @@ def __init__(self, nodeid, multiseq_file, seqtype, conf, confname):
self.confname = confname
self.conf = conf
# Initialize task
task_name = "TCoffee(-%s)" %conf["-mode"] if "-mode" in conf else "TCoffee"
task_name = "TCoffee(-%s)" %conf["-mode"] if "-mode" in conf else "TCoffee" #Ziqi
Copy link
Member

Choose a reason for hiding this comment

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

please remove all #Ziqi tags from final code.

@@ -8,7 +8,8 @@ source:
#git_url: https://github.com/jhcepas/ete.git

build:
noarch_python: True
noarch: python
string: pyh39e3cac_0
Copy link
Member

Choose a reason for hiding this comment

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

why the string?

utils/release.py Outdated
@@ -1,5 +1,6 @@
import re
import commands
#import commands
Copy link
Member

Choose a reason for hiding this comment

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

remove

@@ -691,6 +695,7 @@ def _main(arguments, builtin_apps_path=None):
# setup portable apps
config = {}
for k in apps.builtin_apps:
# concatnate the path with "bin"
Copy link
Member

Choose a reason for hiding this comment

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

concatenate

VERSION Outdated
3.1.2
Copy link
Member

Choose a reason for hiding this comment

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

please, add new line at the end

@@ -54,4 +54,4 @@ def test_01_aa_genetree_worflows_with_model_test(self):
SeqGroup(alg_trimmed)

if __name__ == "__main__":
unittest.main()
unittest.main()
Copy link
Member

Choose a reason for hiding this comment

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

same here. add new line. not a big deal, but it is usually recommended

if arguments[1] == "check":
if not pexist(APPSPATH):
print('this is apps_path', APPSPATH)
Copy link
Member

Choose a reason for hiding this comment

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

this looks as a debug message. Remove or rephrase more formally

@@ -998,7 +1003,7 @@ def _main(arguments, builtin_apps_path=None):
exec_group.add_argument("--nochecks", dest="nochecks",
action="store_true",
help="Skip basic checks (i.e. tools available) everytime the application starts.")

Copy link
Member

Choose a reason for hiding this comment

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

remove empty chars

@@ -29,8 +29,8 @@ def __init__(self, nodeid, alg_phylip_file, constrain_id, model,
base_args["-st"] = "AA" if seqtype == "aa" else "DNA"
else:
if conf[confname]["-st"].startswith("CODON"):
if seqtype == "aa" or "aa" not in GLOBALS["seqtypes"]:
raise ConfigError("IQTREE CODON models require a codon alignmen.\nProvide nucleotide sequences with '-n' and set '--nt-switch-thr 0.0' to ensure codon alignments.")
if seqtype == "aa" or "aa" in GLOBALS["seqtypes"]:
Copy link
Member

Choose a reason for hiding this comment

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

I think it was right in the original version. If codon models are selected, we need to check that alg was done with aa and then translated back into nts.

So, here we expect seqtype = 'nt' and 'aa' in GLOBLALS.

if seqtype is aa or is nt but aa not in globals, raise the Error (which I think is what the IF is covering)

@@ -651,4 +651,4 @@ def cmp(x, y):
"""cmp() exists in Python 2 but was removed in Python 3.
This implements the same behavior on both versions.
"""
return (x > y) - (x < y)
return (x > y) ^ (x < y)
Copy link
Member

Choose a reason for hiding this comment

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

??? this was working before. Why the change? the impact on other functions is high!

@jhcepas jhcepas merged commit b524047 into etetoolkit:master Mar 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants