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

Added heatmap plotting method to Alignment (and tests). #779

Closed
wants to merge 14 commits into from

Conversation

kestrelgorlick
Copy link
Contributor

@jairideout jairideout added this to the 0.2.2: Easy as ABC milestone Nov 25, 2014
@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 5f3b0b8 on Kleptobismol:issue-765 into 3df9544 on biocore:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling cf79a1f on Kleptobismol:issue-765 into 3df9544 on biocore:master.

@gregcaporaso
Copy link
Contributor

Thanks @Kleptobismol, looking forward to trying this out!

@jairideout jairideout self-assigned this Nov 25, 2014
@jairideout
Copy link
Contributor

Thanks @Kleptobismol, I'll take a pass through later today.

@@ -9,7 +9,7 @@
* Added `skbio.stats.power` for performing emperical power analysis. The module uses existing datasets and iteratively draws samples to estimate the number of samples needed to see a significant difference for a given critical value.
* Added `skbio.stats.isubsample` for subsampling from an unknown number of values. This method supports subsampling from multiple partitions and does not require that all items be stored in memory, requiring approximately `O(N*M)`` space where `N` is the number of partitions and `M` is the maximum subsample size.
* Added ``skbio.stats.subsample_counts``, which replaces ``skbio.stats.subsample``. See deprecation section below for more details ([#770](https://github.com/biocore/scikit-bio/issues/770)).

* Added ``heatmap`` method to ``skbio.alignment.Alignment`` for creating a heatmap from an alignment (see [#765](https://github.com/biocore/scikit-bio/issues/765)).
Copy link
Contributor

Choose a reason for hiding this comment

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

add a newline below here to separate this section from the bug fixes section

@ElDeveloper
Copy link
Contributor

This may be a rather general comment but is there a reason why this method doesn't use seaborn to produce the heat map? See this link.

cc @gregcaporaso

@gregcaporaso
Copy link
Contributor

I think using seaborn.heatmap would be a good idea, but that could also be changed in another PR if this is close to merge.

@jairideout
Copy link
Contributor

👍 for using seaborn. Let's get this merged first and then update all of the existing plotting methods to use seaborn in another pull request (Alignment, DissimilarityMatrix, and OrdinationResults).

@coveralls
Copy link

Coverage Status

Coverage increased (+0.0%) when pulling 7f52bcb on Kleptobismol:issue-765 into 3df9544 on biocore:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.15%) when pulling 2e5b21b on Kleptobismol:issue-765 into 3df9544 on biocore:master.

@@ -1668,6 +1671,125 @@ def to_phylip(self, map_labels=False, label_prefix=""):

return '\n'.join(result), new_id_to_old_id

def heatmap(self, value_map, legend_labels=['Minimum', 'Median',
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I didn't catch this before: the default value for legend_labels should be changed to a tuple instead of a list (see here for an explanation why).

@jairideout
Copy link
Contributor

@Kleptobismol done reviewing

@kestrelgorlick
Copy link
Contributor Author

What variable should I be passing to np.empty? It says it requires a "shape" argument (pos1)

@kestrelgorlick
Copy link
Contributor Author

nevermind it's between sequence_length and sequence_count

The docstring entries about the X and Y axis labels are likely in the wrong place.
@kestrelgorlick
Copy link
Contributor Author

What do i do about this?
Warning, treated as error:
WARNING: intersphinx inventory 'http://docs.python.org/dev/objects.inv' not fetchable due to <class 'urllib2.URLError'>: <urlopen error [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed (_ssl.c:581)>

@ebolyen
Copy link
Contributor

ebolyen commented Dec 15, 2014

@Kleptobismol That happens when travis has a network error. I'm rerunning the tests, they should pass fine this time.

@ebolyen
Copy link
Contributor

ebolyen commented Dec 15, 2014

Strange, the python3 test loads the docs fine, but the tests fail. Python2 the tests succeed, but the docs won't load.

@kestrelgorlick
Copy link
Contributor Author

So i didn't do anything wrong?
I am coming to the code sprint around 4 today so maybe we can quickly discuss this there @ebolyen

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) when pulling c29d54f on Kleptobismol:issue-765 into ddbb3dd on biocore:master.

@@ -11,6 +11,7 @@
* Added `skbio.stats.power` for performing empirical power analysis. The module uses existing datasets and iteratively draws samples to estimate the number of samples needed to see a significant difference for a given critical value.
* Added `skbio.stats.isubsample` for subsampling from an unknown number of values. This method supports subsampling from multiple partitions and does not require that all items be stored in memory, requiring approximately `O(N*M)`` space where `N` is the number of partitions and `M` is the maximum subsample size.
* Added ``skbio.stats.subsample_counts``, which replaces ``skbio.stats.subsample``. See deprecation section below for more details ([#770](https://github.com/biocore/scikit-bio/issues/770)).
* Added ``heatmap`` method to ``skbio.alignment.Alignment`` for creating a heatmap from an alignment (see [#765](https://github.com/biocore/scikit-bio/issues/765)).
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please move this under a Features heading in the 0.2.2-dev section?

@jairideout
Copy link
Contributor

@Kleptobismol I took another pass through, please see my comments inline. Thanks!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) when pulling 1cf2e9f on Kleptobismol:issue-765 into ddbb3dd on biocore:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) when pulling 6fa40a8 on Kleptobismol:issue-765 into ddbb3dd on biocore:master.

@jairideout
Copy link
Contributor

@Kleptobismol sorry for the delay on this. I'm pulling your commits to make some minor edits and to play around with the code a bit. I'll submit a new pull request sometime today that has your commits plus my changes.

@jairideout
Copy link
Contributor

Closing in favor of #816.

@jairideout jairideout closed this Feb 11, 2015
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.

add TabularMSA.heatmap method
6 participants