ENH: add Alignment.heatmap method #816

jairideout · 2015-02-11T18:51:43Z

This pull request includes @Kleptobismol's commits in #779. I added a few commits with documentation updates, a few bug fixes, some refactoring, and more unit tests.

Fixes #765.

@gregcaporaso @ebolyen can you please review? @gregcaporaso I'll follow up with a comment on a specific part of the code that I'd like your feedback on.

@gregcaporaso

Fixes scikit-bio#765 Based on @gregcaporaso's scikit-bio cookbook recipe: http://nbviewer.ipython.org/github/biocore/scikit-bio-cookbook/blob/master/Alignment%20viewing%20and%20filtering.ipynb

Also fig_size defaults to None, and so does cmap.

The docstring entries about the X and Y axis labels are likely in the wrong place.

…into aln-heatmap

…additions Took a pass through the code and unit tests, cleaning up the implementation a bit and improving error messages. Also refactored some of the code for easier testing of individual pieces, which uncovered a few bugs: - every value in `value_map` was being used to compute min/median/max for the legend, and default values (if `value_map` was a defaultdict) were ignored - plotting an empty alignment resulted in a cryptic error - plotting an alignment with a single sequence produced the wrong y-axis labels Fixes scikit-bio#765.

jairideout · 2015-02-11T19:00:31Z

skbio/alignment/_alignment.py

+            ``KeyErrors`` are not caught, so all possible values should be in
+            `value_map`, or it should be a ``collections.defaultdict`` which
+            can, for example, default to ``nan``.
+        legend_labels : iterable, optional


@gregcaporaso does the behavior described for legend_labels make sense?

In your original cookbook implementation, all of the values in value_map were used to compute the minimum, median, and maximum. This could produce strange behavior if a value in value_map wasn't used in the heatmap. For example, if value_map had an unused mapping to a maximum value, the "maximum" label wouldn't show up in the legend because a different (smaller) maximum value was used in the heatmap/colorbar.

Also, if a defaultdict was supplied, mappings using the default value weren't being considered when computing min/median/max. For example, suppose the defaultdict had a default value that happened to be the true maximum, the "maximum" label would be placed in the wrong spot.

@jairideout, that makes sense and I see that it has to be that way now that you bring it up. But, I wonder if we should include the values in the legend label as well in this case. So, next to the label for the minimum, you'd always include the min value in parenthesis, and same for mean and max. Otherwise it could be misleading if your labels were *hydrophilic", "medium", and "hydrophobic", and then there were no "hydrophobic" residues in the alignment (I know, very unlikely, but just working from the cookbook example).

That's a good idea, I'll add that in. Since these labels are pretty specialized, I think a better default is to not include these labels on the legend at all (i.e., legend_labels=None). If someone wants to mark the min/median/max, then they have the option to, but then we're not forcing users to label the legend this way. Does that make sense?

Yep, good idea.

coveralls · 2015-02-11T19:06:23Z

Coverage decreased (-0.15%) to 98.72% when pulling 89f45fc on jairideout:aln-heatmap into 1f3bb5a on biocore:master.

jairideout · 2015-02-11T20:10:04Z

@gregcaporaso @ebolyen this is ready for review -- tests are passing now and I manually verified that coverage is at 100% for the code I added (coveralls isn't rerunning for some reason).

coveralls · 2015-02-12T16:00:20Z

Coverage increased (+0.01%) to 98.88% when pulling a7b9094 on jairideout:aln-heatmap into 1f3bb5a on biocore:master.

Conflicts: CHANGELOG.md

coveralls · 2015-02-16T17:59:04Z

Coverage increased (+0.01%) to 98.88% when pulling a97665a on jairideout:aln-heatmap into d9371eb on biocore:master.

ebolyen · 2015-02-20T17:16:20Z

@jairideout Merge conflict.

Conflicts: CHANGELOG.md skbio/alignment/_alignment.py skbio/alignment/tests/test_alignment.py

coveralls · 2015-02-20T21:08:39Z

Coverage increased (+0.01%) to 98.99% when pulling 246b15f on jairideout:aln-heatmap into bf0d8c6 on biocore:master.

jairideout · 2015-02-20T21:19:31Z

Fixed, thanks!

gregcaporaso · 2015-02-25T03:26:49Z

Looks good, thanks @jairideout! I just added one suggestion based on your question - if that doesn't make sense we can discuss tomorrow.

gregcaporaso · 2015-02-25T03:30:04Z

skbio/alignment/_alignment.py

+            See here for choices:
+            http://matplotlib.org/examples/color/colormaps_reference.html
+            If ``None``, defaults to the colormap specified in the user's
+            matplotlibrc file.


What's the default if it's not specified in the user's matplotlibrc?

With any luck, we'll have a better default in the future as mpl is discussing a change to the default color map (matplotlib/matplotlib#875). Anyone want to weigh in on that discussion?

This will fall back to whatever matplotlib's default is (there is always a "base" config file included in a matplotlib install, similar to how QIIME config files work). I don't think listing this here is appropriate because matplotlib's defaults may change and we won't want to update these docs when that happens.

Yep, agree.

jairideout · 2015-03-03T18:00:20Z

Thanks for reviewing @gregcaporaso! I had another question (see inline). In the meantime I'll work on the change you suggested.

@gregcaporaso

Based on @gregcaporaso's suggestions.

jairideout · 2015-03-05T19:00:08Z

@gregcaporaso I made the changes we discussed. Can you and @ebolyen review?

ebolyen · 2015-03-09T20:57:34Z

skbio/alignment/tests/test_alignment.py

+    def test_heatmap_invalid_sequence_order(self):
+        # duplicate ids
+        with self.assertRaises(ValueError):
+            self.a1.heatmap({}, sequence_order=['d1', 'd2', 'd1'])


Can you assert some basic verbs in the error message exist to ensure that the correct error is being raised?

Excellent idea!

jairideout · 2015-04-02T15:31:37Z

Thanks for reviewing @ebolyen! I'm going to hold off on addressing your comments until after the alignment object is refactored (#823) , then I'll update the changes here to work with the new API. I'm marking this as "do not merge" for now.

kestrelgorlick and others added 18 commits November 24, 2014 17:10

Added heatmap plotting method to Alignment (and tests).

426a3f6

Fixes scikit-bio#765 Based on @gregcaporaso's scikit-bio cookbook recipe: http://nbviewer.ipython.org/github/biocore/scikit-bio-cookbook/blob/master/Alignment%20viewing%20and%20filtering.ipynb

Addresses @Jorge-C's comment regarding matplotlib.pyplot import.

c6c8d6e

Added my alignment heatmap method to the changelog.

5f9d989

Fixes Travis plotting errors.

5f3b0b8

Cleaned up docstring.

cf79a1f

Addresses @jairideout's comments.

fc6015c

Also fig_size defaults to None, and so does cmap.

Fixes Travis errors.

d51d603

Fixes Travis error.

7f52bcb

Cleaned up tests and added a few lines to _alignment.py .

2e5b21b

Addresses @jairideout's comments

42989b7

The docstring entries about the X and Y axis labels are likely in the wrong place.

Fixed Travis error

6ab20c0

Merge branch 'master' into issue-765

c29d54f

Addresses @jairideout's comments

1cf2e9f

Adds tests in order to fix coverage loss.

6fa40a8

Merge branch 'issue-765' of https://github.com/Kleptobismol/scikit-bio …

b50ecc0

…into aln-heatmap

DOC: move Alignment.heatmap changelog mention under 0.2.2-dev heading

c8e83f4

DOC: minor cleanup to Alignment.heatmap docstring

0e2aabd

jairideout added this to the 0.2.3: Easy as ABC milestone Feb 11, 2015

jairideout assigned gregcaporaso Feb 11, 2015

jairideout added the feature-enhancement label Feb 11, 2015

jairideout mentioned this pull request Feb 11, 2015

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

Closed

jairideout reviewed Feb 11, 2015
View reviewed changes

BUG: fix Python 3 error resulting from incorrect creation of numpy array

a7b9094

jairideout removed the feature-enhancement label Feb 12, 2015

jairideout added 2 commits February 16, 2015 10:32

Merge branch 'master' into aln-heatmap

2683103

Conflicts: CHANGELOG.md

MAINT: stop forcing agg backend

a97665a

jairideout added 2 commits February 20, 2015 13:25

Merge branch 'master' into aln-heatmap

6e58588

Conflicts: CHANGELOG.md skbio/alignment/_alignment.py skbio/alignment/tests/test_alignment.py

BUG/TST: add back missing npt import

246b15f

gregcaporaso reviewed Feb 25, 2015
View reviewed changes

jairideout added 2 commits March 5, 2015 08:57

Merge branch 'master' into aln-heatmap

828925a

ENH: default to no legend labels; report stats in legend labels

cc68f7c

Based on @gregcaporaso's suggestions.

jairideout modified the milestones: 0.3.0: Sequences, Collections, and Alignments, oh my!, 0.3.4: Easy as ABC Mar 9, 2015

ebolyen reviewed Mar 9, 2015
View reviewed changes

jairideout unassigned gregcaporaso Apr 2, 2015

jairideout added the DO NOT MERGE label Apr 2, 2015

jairideout removed this from the 0.3.0: Sequences, Collections, and Alignments, oh my! milestone Jun 15, 2015

jairideout closed this Jun 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: add Alignment.heatmap method #816

ENH: add Alignment.heatmap method #816

jairideout commented Feb 11, 2015

jairideout Feb 11, 2015

gregcaporaso Feb 25, 2015

jairideout Mar 3, 2015

gregcaporaso Mar 3, 2015

coveralls commented Feb 11, 2015

jairideout commented Feb 11, 2015

coveralls commented Feb 12, 2015

coveralls commented Feb 16, 2015

ebolyen commented Feb 20, 2015

coveralls commented Feb 20, 2015

jairideout commented Feb 20, 2015

gregcaporaso commented Feb 25, 2015

gregcaporaso Feb 25, 2015

gregcaporaso Feb 26, 2015

jairideout Mar 3, 2015

gregcaporaso Mar 3, 2015

jairideout commented Mar 3, 2015

jairideout commented Mar 5, 2015

ebolyen Mar 9, 2015

jairideout Mar 9, 2015

jairideout commented Apr 2, 2015

ENH: add Alignment.heatmap method #816

ENH: add Alignment.heatmap method #816

Conversation

jairideout commented Feb 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 11, 2015

jairideout commented Feb 11, 2015

coveralls commented Feb 12, 2015

coveralls commented Feb 16, 2015

ebolyen commented Feb 20, 2015

coveralls commented Feb 20, 2015

jairideout commented Feb 20, 2015

gregcaporaso commented Feb 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jairideout commented Mar 3, 2015

jairideout commented Mar 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jairideout commented Apr 2, 2015