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

display the colormaps in docs #33

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

chrishavlin
Copy link
Contributor

@chrishavlin chrishavlin commented Jun 13, 2024

Hi there!

I spent a few mins just now plotting up all the colormaps just to check out what they're like, and then saw you're using myst-markdown for you docs, so figured it'd be easy enough to send along a PR to display all the colormaps in your docs.

One question: I added the display to the existing # Reference section of api.md. It makes the page rather long and I could instead add a gallery.md page with just the visuals. Or I could modify the docstrings for the cm and cm_colorblind to remove the lists so the info isn't repeated. Or keep it as is in this PR. Any preference? I'm inclined to add a gallery.md page, but thought I'd ask first :)

@zssherman
Copy link
Collaborator

@chrishavlin Thanks for the PR! I could see it being fine in reference! My one question would be in the doc build i'm seeing the colormaps shrink as they go down the docs, was wondering if it is a possible bug?:
https://cmweather--33.org.readthedocs.build/en/33/api.html

@chrishavlin
Copy link
Contributor Author

in the doc build i'm seeing the colormaps shrink as they go down the docs, was wondering if it is a possible bug?:

oh wow, no idea how your eyes caught that... but it does seem like some of them are a few pixels shorter. I had to pull open some screenshots in photoshop to convince myself it wasn't a weird optical illusion :) I'll try wrapping them all in extra div tags with scaling to normalize them (but may not get to it till tomorrow or Monday). Also will fix the style failures (didn't bother setting up pre-commit locally...).

@zssherman
Copy link
Collaborator

zssherman commented Jun 13, 2024

@chrishavlin So weirdly enough on mac it was doing it for me. But on my windows machine i checked, it looks fine to me haha! I have no idea what happened so could have been so weird rendering in the browser on mac for some reason. Those markers that say "under bad over" or those automatically being generated? And no worries as well!

@zssherman
Copy link
Collaborator

Screenshot (336)

@chrishavlin
Copy link
Contributor Author

chrishavlin commented Jun 13, 2024

Ya,the under/bad/over are automatic. It's pulling the _repr_html_ attribute for each color map (which the base matplotlib color map object defines) and displaying them in ipython HTML objects. Happy to see if I can remove those labels easily.

@chrishavlin
Copy link
Contributor Author

quick followup -- i just checked it out on my mac and in firefox it looks correct but in safari I'm seeing what you described (and it makes much more sense than the small pixel-scale differences I was looking at yesterday haha). in any case, i think i can fix it by modifying some div tags, will give it a go!

@zssherman
Copy link
Collaborator

@chrishavlin Ah makes sense, thanks for confirming! Yeah that sounds good to me! Thanks again!

@zssherman zssherman self-requested a review June 15, 2024 00:43
@chrishavlin
Copy link
Contributor Author

Sorry for the delay -- about to push up a change that fixes the safari display issue for me locally and removes the over/under/bad labels. To do this I added a new function (and a new test) to cmweather.cm: _get_cmap_gallery_html. The only other options I could think of were to: (1) use some string manipulation of the html str output by the base matplotlib.colors.Colormap._get_html_repr_ or (2) create a subclass of the LinearSegmentedColormap to over-ride _get_html_repr and use everywhere within cmweather. The first option seemed potentially brittle (it could break if matplotlib changed its formatting slightly) and the second option of a new class seemed too invasive for the fairly simple goal of this PR. So I went with the new function.

@zssherman
Copy link
Collaborator

@chrishavlin No worries and thanks for the update! This looks good to me! @mgrover1 If you have time for a final review, if you could review as well.

@mgrover1
Copy link
Collaborator

Great work here @chrishavlin - merging away!

@mgrover1 mgrover1 merged commit 07793fc into openradar:main Jun 21, 2024
6 checks passed
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.

3 participants