Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Nichesort #16

Merged
merged 11 commits into from
Jul 27, 2016
Merged

Nichesort #16

merged 11 commits into from
Jul 27, 2016

Conversation

mortonjt
Copy link
Collaborator

Depends on #12.

Used to generate band tables when the gradient is known.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 99.371% when pulling 3d9ded6 on mortonjt:nichesort into f79d9a6 on biocore:master.

@mortonjt mortonjt mentioned this pull request Jul 25, 2016
2 tasks
@josenavas
Copy link
Member

@mortonjt can you pull from upstream/master?

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 99.342% when pulling 42221f5 on mortonjt:nichesort into ff3df22 on biocore:master.

@@ -0,0 +1,81 @@
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

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

Missing license notice.

@josenavas
Copy link
Member

Some comments. Thanks @mortonjt

np.float :
The mean gradient that the organism lives in.
"""
if len(abundances) != len(gradient):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't feel strongly about this but normally we do something like this to minimize how many times you calculate the lenght of the iterables:

len_abundances = len(abundances)
len_gradient = len(gradient)
if len_abundances != len_gradient:
    ....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@antgonza
Copy link
Contributor

some more comments.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 99.338% when pulling 23acedf on mortonjt:nichesort into ff3df22 on biocore:master.

@mortonjt
Copy link
Collaborator Author

Thanks for taking the time to review this! All comments have been addressed.

# normalizes the proportions of the organism across all of the
# samples to add to 1.
v = abundances / abundances.sum()
m = np.dot(gradient, v)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line really doing this:
E[g | x] = \frac{1}{\sum\limits_{j=1}^N x_j} \sum\limits_{i=1}^N x_i g_i

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup

On Jul 27, 2016 11:56 AM, "Antonio Gonzalez" notifications@github.com
wrote:

In gneiss/sort.py
#16 (comment):

  • ValueError:
  •    If the length of `gradient` contains nans.
    
  • """
  • len_abundances = len(abundances)
  • len_gradient = len(gradient)
  • if len_abundances != len_gradient:
  •    raise ValueError("Length of `abundances` (%d) doesn't match the length"
    
  •                     " of the `gradient` (%d)" % (len_abundances,
    
  •                                                  len_gradient))
    
  • if np.any(pd.isnull(gradient)):
  •    raise ValueError("`gradient` cannot have any nans.")
    
  • normalizes the proportions of the organism across all of the

  • samples to add to 1.

  • v = abundances / abundances.sum()
  • m = np.dot(gradient, v)

Is this line really doing this:
E[g | x] = \frac{1}{\sum\limits_{j=1}^N x_j} \sum\limits_{i=1}^N x_i g_i


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/biocore/gneiss/pull/16/files/23acedfd2291c0b3716023c9867c2629b1c16dc4#r72501098,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AD_a3cfBc1UiQuuaW3PR3kUGYTYwh3Fcks5qZ6m_gaJpZM4JUnpZ
.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mortonjt and I discussed offline and realized that the code and the formula are similar but not the same, one was a simplification of the other. @mortonjt fixed it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 99.346% when pulling 940d7e8 on mortonjt:nichesort into ff3df22 on biocore:master.

pd.DataFrame :
Sorted table according to the gradient of the samples, and the niches
of the organisms along that gradient.
"""
Copy link
Member

Choose a reason for hiding this comment

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

missing raises section

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Doh. Added

@josenavas
Copy link
Member

Thanks @mortonjt ! The comments really help to fully understand what is going on in the code! Just a minor comment about missing a section in the docs and I think this is good to go!

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 99.346% when pulling 7ee169c on mortonjt:nichesort into ff3df22 on biocore:master.

@josenavas
Copy link
Member

Good to merge - I just realized that I don't have merge privileges here

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 99.346% when pulling 0168b42 on mortonjt:nichesort into ff3df22 on biocore:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 99.346% when pulling c9e2373 on mortonjt:nichesort into ff3df22 on biocore:master.

@antgonza antgonza merged commit 8f2d1a7 into biocore:master Jul 27, 2016
@mortonjt
Copy link
Collaborator Author

Thanks for your insights on this! This feedback is seriously improving the usability of this module.

This was referenced Jul 28, 2016
@wasade wasade mentioned this pull request Aug 4, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants