search

phylo42 / PEWO

Phylogenetic Placement Evaluation Workflows : Benchmark placement software and different reference trees
MIT License
12 stars 9 forks source link

App-SpaM #4

Closed matthiasblanke closed 3 years ago

matthiasblanke commented 3 years ago

Hi everybody, finally we have cleaned up App-SpaM and added it to bioconda. Hence, we would also appreciate to have it added to PEWO. AFAIK I followed all developer instructions, if you find any errors I should fix just let me know. I also had to make some small adjustments in the PEWO_java submodule but couldn't figure out how to include them here - should I just do another pull request at the other repo?

blinard-BIOINFO commented 3 years ago

Dear Matthias, Thank you for this ! Really appreciated ! We will carefully review your code and merge it afterwards.

blinard-BIOINFO commented 3 years ago

@matthiasblanke Concerning the java side, yes please, do a pull request to this other repo. I'm just surprisd you had to do something there, it's intended to be independant. What was the nature of the change ?

matthiasblanke commented 3 years ago

Thanks for the fast response. I did another PR at the PEWO_java repo with my changes - maybe they are unnecessary or there is a better way to do it though... If you need any help or more info please let me know.

blinard-BIOINFO commented 3 years ago

Oh, I see. Indeed, I will have to change these few lines that were initially for debugging purposes... Thank you for pointing it !

nromashchenko commented 3 years ago

Hello, Good job! I'd just ask one more thing... Our Travis CI runs two pipelines travis/tests/1_..., travis/tests/2_... on every push, making sure it's possible to build and run those toy examples in the isolated environment. In your request you did not change config files of those, and App-SpaM is not tested with CI. "All checks have passed" does not tell anything about App-SpaM yet. Could you please add App-SpaM there as well? Do not forget to include App-SpaM in test_soft. Thanks

matthiasblanke commented 3 years ago

Ah, I didn't notice those.. added it to the two config files; looks like it worked?

blinard-BIOINFO commented 3 years ago

Our Travis CI runs two pipelines travis/tests/1_..., travis/tests/2_... on every push, making sure it's possible to build and run those toy examples in the isolated environment. In your request you did not change config files of those, and App-SpaM is not tested with CI.

Good point, we need to add this in the developer documentation. I'm adding an issue for this.