Skip to content

Add getters and _repr_html_ for over/under/bad values of Colormap objects. #17900

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

Merged
merged 20 commits into from
Aug 19, 2020

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Jul 12, 2020

PR Summary

This PR extends from #17888 to add over/under/bad values in the _repr_html_ of matplotlib.colors.Colormap objects. This also adds public getters for those values, which previously had only setters.

Demo

image

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/next_api_changes/* if API changed in a backward-incompatible way

@bdice bdice marked this pull request as ready for review July 12, 2020 19:19
@bdice bdice force-pushed the colormap-repr-under-over-bad branch from 1c633ab to cc8318e Compare July 12, 2020 19:21
@tacaswell tacaswell added this to the v3.4.0 milestone Jul 12, 2020
@jklymak
Copy link
Member

jklymak commented Jul 12, 2020

This is what I wondered about in the first PR. over/under are usually in the extended portion of the colorbar which we represent via a triangle on either side:

https://matplotlib.org/2.0.2/_images/colorbar_only.png

I also wonder if the colorbar needs to be so wide, but that is just an aesthetic choice

@efiring
Copy link
Member

efiring commented Jul 12, 2020

@jklymak I think that the use of color blocks for over, under, and bad is a good idea. It is explicit and complete. Using the actual colorbar with extensions would show under and over, but not bad.

@jklymak
Copy link
Member

jklymak commented Jul 12, 2020

ThTs fine. I'm certainly not going to block on this at all. I just think it's a little inconsistent.

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

I think this is a great addition, and will be very useful to check interactively what is currently set for the colormap values!

For the ordering, I am wondering if there is a way to put the "under" box left-aligned and the "over" box right-aligned to put them next to the values they are "near"? Right now, the yellow "over" is a bit far away from the far right edge. (That might mean putting the title on a row above the squares to squeeze it in) I missed the sprint unfortunately, so missed if you already tried this or not.

@bdice
Copy link
Contributor Author

bdice commented Jul 13, 2020

For the ordering, I am wondering if there is a way to put the "under" box left-aligned and the "over" box right-aligned to put them next to the values they are "near"? Right now, the yellow "over" is a bit far away from the far right edge. (That might mean putting the title on a row above the squares to squeeze it in) I missed the sprint unfortunately, so missed if you already tried this or not.

@greglucas I discussed with @tacaswell and we decided it would be less obtrusive in a notebook if we kept the color map name and under/over/bad boxes on the same line (above the image). Are you suggesting one of layouts B or C? (represented in ASCII style)

Layout A (current):

viridis  under: [█]  over: [█]  bad: [█]
[████████████████████████████████████████]

Layout B: left/right aligned with text labels

viridis                           bad: [█]
[████████████████████████████████████████]
under: [█]                       over: [█]

Layout C: left/right aligned with color swatches

viridis                            bad [█]
[████████████████████████████████████████]
[█] under                         over [█]

@bdice bdice requested a review from timhoffm July 13, 2020 00:10
@greglucas
Copy link
Contributor

Nice, I think it is OK the way it is and I haven't messed with anything else to see how they look. My thought was something along the lines of your third option. Maybe like this?

                viridis
[████████████████████████████████████████]
[█] under       bad [█]           over [█]

I agree though, this would add more height to the image, so maybe not as ideal as having them all on the same line.

@jklymak
Copy link
Member

jklymak commented Jul 13, 2020

Or you could have over/under beside the colorbar (perhaps as triangles) and the bad swatch beside the title. I'd put the title on the left and the bad swatch on the right.

viridis                            bad [█]
< [████████████████████████████████████████] >

@jklymak
Copy link
Member

jklymak commented Jul 13, 2020

Something like this adds a triangle (if you just want to use html/css: https://stackoverflow.com/questions/44109039/how-to-put-a-css-triangle-before-a-span

@bdice
Copy link
Contributor Author

bdice commented Jul 14, 2020

@jklymak @greglucas I like the idea, but a triangle implemented with CSS border properties will not work with the thin grey line that denotes the edge of the color map image (which also uses border). I don't think you could put the thin grey line around the triangle. @tacaswell suggested adding that border because some color maps may fade to white (or transparent) on one end, and showing the boundary of the image helps with that.

@greglucas's suggestion of

                viridis
[████████████████████████████████████████]
[█] under       bad [█]           over [█]

is a viable design, if there is consensus on this being the best option. I am happy to make another revision if we get a stronger consensus on the design.

@dopplershift
Copy link
Contributor

I feel like we're treading a tad close to "design by committee" and bike-shedding to death here. I'm 👍 to @greglucas's suggestion.

@bdice bdice force-pushed the colormap-repr-under-over-bad branch from 8a36e2a to c43cceb Compare August 9, 2020 03:47
@bdice
Copy link
Contributor Author

bdice commented Aug 9, 2020

@dopplershift @jklymak @greglucas @timhoffm I spent some time trying to make the design mentioned above and got as close as I could. The alignment of the labels below the image is very tricky. Is the image below acceptable (or is someone else with CSS expertise able to help)?

Revised appearance

image

@bdice
Copy link
Contributor Author

bdice commented Aug 9, 2020

I would also appreciate help understanding why CircleCI docs-python3* is failing. I don't see any error messages in the CI log...?

@jklymak
Copy link
Member

jklymak commented Aug 9, 2020

We fail on sphinx warnings as well

/home/circleci/project/doc/users/next_whats_new/colormap_get_under_over_bad.rst:4: WARNING: py:obj reference target not found: colors.Colormap.get_under
/home/circleci/project/doc/users/next_whats_new/colormap_get_under_over_bad.rst:4: WARNING: py:obj reference target not found: colors.Colormap.get_over
/home/circleci/project/doc/users/next_whats_new/colormap_get_under_over_bad.rst:4: WARNING: py:obj reference target not found: colors.Colormap.get_bad

@greglucas
Copy link
Contributor

Personally, I would go with either your first version (with the boxes on the same line as the name), or with the version below. I think the thing you were missing was to flex the div and justify the contents of the container. 'display:flex; justify-content:space-between;

With that I get this image:
Screen Shot 2020-08-09 at 3 17 42 PM

Click for code

        def color_block(color):
            hex_color = to_hex(color, keep_alpha=True)
            return ('<div title="' + hex_color + '" ' +
                    'style="display: inline-block; ' +
                    'width: 1em; height: 1em; ' +
                    'margin: 0; ' +
                    'vertical-align: middle; ' +
                    'border: 1px solid #555; ' +
                    'background-color: ' + hex_color + ';">' +
                    '</div>')

        return ('<div style="vertical-align: middle;">' +
                '<strong>' + self.name + '</strong> ' +
                '</div>' +
                '<div id="cmap"><img ' +
                'alt="' + self.name + ' color map" ' +
                'title="' + self.name + '"' +
                'style="border: 1px solid #555;" ' +
                'src="data:image/png;base64,' + png_base64 + '"></div>' +
                '<div style="vertical-align: middle; width:402px; ' +
                'display:flex; justify-content:space-between;">' +
                '<div style="float:left;">' +
                color_block(self.get_under()) + ' under' +
                '</div>' +
                '<div style="margin:0 auto; display:inline-block;">' +
                'bad ' + color_block(self.get_bad()) +
                '</div>' +
                '<div style="float: right;">' +
                'over ' + color_block(self.get_over()) +
                '</div>')

@bdice
Copy link
Contributor Author

bdice commented Aug 10, 2020

Thanks @greglucas and @jklymak, that helped a lot. I also got the content to be responsive in narrow windows (max-width instead of width). This PR should be ready for final review! 😄

image

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Do you want to add a test for the _repr_html_ function? Assert that it is returning the html expected in a specific case or something similar.

Looks good to me! Only the few minor questions.

@bdice
Copy link
Contributor Author

bdice commented Aug 16, 2020

Do you want to add a test for the _repr_html_ function? Assert that it is returning the html expected in a specific case or something similar.

@greglucas I improved the HTML tests. Checking against a specific output is probably not ideal because the PNG is directly embedded as base64 and the HTML output for viridis is ~2,104 characters. It wouldn't fit well into a test file.

<div style="vertical-align: middle;"><strong>viridis</strong> </div><div class="cmap"><img alt="viridis color map" title="viridis" style="border: 1px solid #555;" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAgAAAABACAYAAABsv8+/AAAAF3RFWHRUaXRsZQB2aXJpZGlzIGNvbG9yIG1hcCbEUIsAAAAddEVYdERlc2NyaXB0aW9uAHZpcmlkaXMgY29sb3IgbWFwD7q3ogAAAEt0RVh0QXV0aG9yAE1hdHBsb3RsaWIgdjMuMy4wcmMxLnBvc3Q2NDMuZGV2MCtnNWIwOTVmOTNhLCBodHRwczovL21hdHBsb3RsaWIub3JnpWoE0wAAAE10RVh0U29mdHdhcmUATWF0cGxvdGxpYiB2My4zLjByYzEucG9zdDY0My5kZXYwK2c1YjA5NWY5M2EsIGh0dHBzOi8vbWF0cGxvdGxpYi5vcmdUElJHAAACIklEQVR4nO3WQZKbMBRF0S/YWpaQ/S8l9CAyFAIZx5XZO2fikvQRVA+6bvvVfm9VVdVa/f1d+s95XX3dXnNL3x/Wx/n98+N7LvfO7r/ce/+7PZzPnt/28/ryvE737/Ot7ueG+8b563q8f3ZvDc8N9ywP55fn37/nef3p/ffrb9/zv+9/nKv3c5/vb989X0/z21ffMZ4fc5Pz/Ttmz7/fb5O5dlkPz9U4N/xbm64f9vu9y2V+6/s1rMfz+3tmc/tvPZwPc0v78+HceX795/Pze9a6f++4v76eq4f1673D+67r++847j1//zF/vu86P7xvtj98x+x71xr/Lue/53rZr75f5/W+34b9vu77+7pPHPNL/+1zbTndAwAEEQAAEEgAAEAgAQAAgQQAAAQSAAAQSAAAQCABAACBBAAABBIAABBIAABAIAEAAIEEAAAEEgAAEEgAAEAgAQAAgQQAAAQSAAAQSAAAQCABAACBBAAABBIAABBIAABAIAEAAIEEAAAEEgAAEEgAAEAgAQAAgQQAAAQSAAAQSAAAQCABAACBBAAABBIAABBIAABAIAEAAIEEAAAEEgAAEEgAAEAgAQAAgQQAAAQSAAAQSAAAQCABAACBBAAABBIAABBIAABAIAEAAIEEAAAEEgAAEEgAAEAgAQAAgQQAAAQSAAAQSAAAQCABAACBBAAABBIAABBIAABAIAEAAIEEAAAEEgAAEEgAAECgH25wiYfFwpm0AAAAAElFTkSuQmCC"></div><div style="vertical-align: middle; max-width: 514px; display: flex; justify-content: space-between;"><div style="float: left;"><div title="#440154ff" style="display: inline-block; width: 1em; height: 1em; margin: 0; vertical-align: middle; border: 1px solid #555; background-color: #440154ff;"></div> under</div><div style="margin: 0 auto; display: inline-block;">bad <div title="#00000000" style="display: inline-block; width: 1em; height: 1em; margin: 0; vertical-align: middle; border: 1px solid #555; background-color: #00000000;"></div></div><div style="float: right;">over <div title="#fde725ff" style="display: inline-block; width: 1em; height: 1em; margin: 0; vertical-align: middle; border: 1px solid #555; background-color: #fde725ff;"></div></div></div>

@bdice bdice requested a review from timhoffm August 16, 2020 03:02
@timhoffm
Copy link
Member

timhoffm commented Aug 18, 2020

@QuLogic is there a reason you did not merge?

Should be squashed to one or a few commits.

@QuLogic
Copy link
Member

QuLogic commented Aug 18, 2020

I restarted tests, I believe. Probably the macOS/TkAgg issue.

@timhoffm timhoffm merged commit 13e3573 into matplotlib:master Aug 19, 2020
@timhoffm
Copy link
Member

@bdice Thanks for your contribution and the patience to work though the various review iterations! We hope to see you back some time.

@bdice bdice deleted the colormap-repr-under-over-bad branch August 19, 2020 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants