search

biocore / qiime

Official QIIME 1 software repository. QIIME 2 (https://qiime2.org) has succeeded QIIME 1 as of January 2018.
GNU General Public License v2.0
286 stars 267 forks source link

remove dependence on cogent.phylo.nj.nj (neighbor joining) #1552

Closed gregcaporaso closed 10 years ago

gregcaporaso commented 10 years ago

looking for this in scipy now

gregcaporaso commented 10 years ago

The nj code in PyCogent defines it's own custom objects, and has UI-specific progress indicators, etc, so it'd be a big headache to port.

Does anyone know if one of these hierarchical clustering methods in scipy is neighbor joining?

This could be another good candidate to drop until we have a better underlying implementation as it's only used in one script (neighbor_joining.py, which isn't called by any workflows) and we also have UPGMA, which we can use from scipy.

gregcaporaso commented 10 years ago

@justin212k, do you have thoughts on this one?

gregcaporaso commented 10 years ago

We may end up needing to re-write this. This page has some good info comparing a few diffrerent implementations, including cogent's and phylip's nj, which produce the same result.

justin212k commented 10 years ago

Sounds reasonable to me. There's a faster but less exact variant I worked on way way back in the day ( https://github.com/justin212k/cogent_rnj/tree/master) . But also uses much pycogent, so perhaps not useful.

On Thu, May 15, 2014 at 10:35 AM, Greg Caporaso notifications@github.comwrote:

We may end up needing to re-write this. This pagehttp://telliott99.blogspot.com/2010/11/my-version-of-neighbor-joining-in.htmlhas some good info comparing a few diffrerent implementations, including cogent's and phylip's nj, which produce the same result.

— Reply to this email directly or view it on GitHubhttps://github.com/biocore/qiime/issues/1552#issuecomment-43217349 .

gregcaporaso commented 10 years ago

Thanks @justin212k, that code is better commented too, so a good starting point. Any chance you have bandwidth to working on removing the PyCogent dependency from it (looks pretty minimal - could probably be ported to use the skbio TreeNode and DistanceMatrix objects) and contributing it to skbio? Doesn't have to be right away, but probably within the next two-three weeks to be useful. If not, would you be willing to give us permission to include your code in skbio under BSD license and do the refactor ourselves?

justin212k commented 10 years ago

BSD permission of course given. I'll take a stab at getting it done and either have it done by may 21, or apologize and not do it, as chaos shall likely reign in my life after that point.

On Thu, May 15, 2014 at 12:20 PM, Greg Caporaso notifications@github.comwrote:

Thanks @justin212k https://github.com/justin212k, that code is better commented too, so a good starting point. Any chance you have bandwidth to working on removing the PyCogent dependency from it (looks pretty minimal - could probably be ported to use the skbio TreeNode and DistanceMatrix objects) and contributing it to skbio? Doesn't have to be right away, but probably within the next two-three weeks to be useful. If not, would you be willing to give us permission to include your code in skbio under BSD license and do the refactor ourselves?

— Reply to this email directly or view it on GitHubhttps://github.com/biocore/qiime/issues/1552#issuecomment-43231652 .

gregcaporaso commented 10 years ago

Awesome, thanks!

gregcaporaso commented 10 years ago

So given this, we're not going to worry about removing that QIIME dependency at the moment since this solution is in progress. Listing this issue number for nj in The Spreadsheet.

gregcaporaso commented 10 years ago

@justin212k, just wanted to check in on this. Let us know what you think about getting it in.

justin212k commented 10 years ago

Haven't done it yet, but think I can in the next few days. Sorry to string you along.

On May 21, 2014 6:57 AM, "Greg Caporaso" notifications@github.com wrote:

@justin212k https://github.com/justin212k, just wanted to check in on this. Let us know what you think about getting it in.

— Reply to this email directly or view it on GitHubhttps://github.com/biocore/qiime/issues/1552#issuecomment-43757319 .

gregcaporaso commented 10 years ago

No prob, just want to make sure someone is working on it. Next few days is sooner than I'd be able to get to it myself.

On Wed, May 21, 2014 at 5:36 PM, justin212k notifications@github.comwrote:

Haven't done it yet, but think I can in the next few days. Sorry to string you along.

On May 21, 2014 6:57 AM, "Greg Caporaso" notifications@github.com wrote:

@justin212k https://github.com/justin212k, just wanted to check in on this. Let us know what you think about getting it in.

Reply to this email directly or view it on GitHub< https://github.com/biocore/qiime/issues/1552#issuecomment-43757319> .

Reply to this email directly or view it on GitHubhttps://github.com/biocore/qiime/issues/1552#issuecomment-43834869 .

justin212k commented 10 years ago

Hi folks, I've got some neighbor-joining code put together, but it's still quite rough (https://github.com/justin212k/scikit-bio/tree/neighbor_joining). It's not relaxed neighbor joining, but I think that's fine for our purposes.

I wish I had more time to clean it up, but I also was divested of my last development computer, so I'm typing things on websites now. And I'm fleeing the lower 48. So if the code is useful (and I think it's not so ugly as to be useless), I'd love to have someone clean it up and put it into skbio.

On Wed, May 21, 2014 at 8:35 PM, Greg Caporaso notifications@github.com wrote:

No prob, just want to make sure someone is working on it. Next few days is sooner than I'd be able to get to it myself.

On Wed, May 21, 2014 at 5:36 PM, justin212k <notifications@github.com

wrote:

Haven't done it yet, but think I can in the next few days. Sorry to string you along.

On May 21, 2014 6:57 AM, "Greg Caporaso" notifications@github.com wrote:

@justin212k https://github.com/justin212k, just wanted to check in on

this. Let us know what you think about getting it in.

Reply to this email directly or view it on GitHub< https://github.com/biocore/qiime/issues/1552#issuecomment-43757319> .

Reply to this email directly or view it on GitHub< https://github.com/biocore/qiime/issues/1552#issuecomment-43834869>

.

— Reply to this email directly or view it on GitHub https://github.com/biocore/qiime/issues/1552#issuecomment-43845424.

gregcaporaso commented 10 years ago

Thanks @justin212k, I can take next pass. Could you issue a pull request, and note DO NOT MERGE in the comment? That makes review a lot more convenient.

On Sun, Jun 1, 2014 at 6:07 PM, justin212k notifications@github.com wrote:

Hi folks, I've got some neighbor-joining code put together, but it's still quite rough ( https://github.com/justin212k/scikit-bio/tree/neighbor_joining). It's not relaxed neighbor joining, but I think that's fine for our purposes.

I wish I had more time to clean it up, but I also was divested of my last development computer, so I'm typing things on websites now. And I'm fleeing the lower 48. So if the code is useful (and I think it's not so ugly as to be useless), I'd love to have someone clean it up and put it into skbio.

On Wed, May 21, 2014 at 8:35 PM, Greg Caporaso notifications@github.com wrote:

No prob, just want to make sure someone is working on it. Next few days is sooner than I'd be able to get to it myself.

On Wed, May 21, 2014 at 5:36 PM, justin212k <notifications@github.com

wrote:

Haven't done it yet, but think I can in the next few days. Sorry to string you along.

On May 21, 2014 6:57 AM, "Greg Caporaso" notifications@github.com wrote:

@justin212k https://github.com/justin212k, just wanted to check in on

this. Let us know what you think about getting it in.

Reply to this email directly or view it on GitHub< https://github.com/biocore/qiime/issues/1552#issuecomment-43757319> .

Reply to this email directly or view it on GitHub< https://github.com/biocore/qiime/issues/1552#issuecomment-43834869>

.

Reply to this email directly or view it on GitHub https://github.com/biocore/qiime/issues/1552#issuecomment-43845424.

Reply to this email directly or view it on GitHub https://github.com/biocore/qiime/issues/1552#issuecomment-44793551.

gregcaporaso commented 10 years ago

skbio.core.tree.nj is now available, so we should be able to update QIIME to remove this cogent dependency.