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 cividis, a new colormap mathematically optimized while consider… #2178

Merged
merged 4 commits into from
Nov 27, 2017

Conversation

jamienunez
Copy link
Contributor

…ing those with a color vision deficiency. Paper going through the review process: Nuñez J, Anderton C, and Renslow R. Optimizing colormaps with consideration for color vision deficiency to enable accurate interpretation of scientific data. 2018.

Thanks for your interest in plotly.js!

Developers are strongly encouraged to first make a PR to their own plotly.js fork and ask one of the maintainers to review the modifications there. Once the pull request is deemed satisfactory, the developer will be asked to make a pull request to the main plotly.js repo and may be asked to squash some commits before doing so.

Before opening a pull request, developer should:

  • git rebase their local branch off the latest master,
  • make sure to not git add the dist/ folder (the dist/ is updated only on verion bumps),
  • write an overview of what the PR attempts to do,
  • select the Allow edits from maintainers option (see this article for more details).

Note that it is forbidden to force push (i.e. git push -f) to remote branches associated with opened pull requests. Force pushes make it hard for maintainers to keep track of updates. Therefore, if required, please git merge master into your PR branch instead of git rebase master.

@etpinard
Copy link
Contributor

Can you share a screenshot of what that new colorscale looks like?

@alexcjohnson
Copy link
Collaborator

The test failure is just from using hard tabs - I'm surprised that this is breaking at the build step rather than syntax checking, but whatever, we use 4 spaces as a tab.

I guess Cividis is this: http://forum.imagej.net/t/new-lut-for-fiji-cividis/7467 - looks useful, I'd be happy to have it in plotly.js. We'll need a test image for it, ala https://github.com/plotly/plotly.js/blob/master/test/image/mocks/viridis_heatmap.json and baseline image https://github.com/plotly/plotly.js/blob/master/test/image/baselines/viridis_heatmap.png

@jamienunez
Copy link
Contributor Author

Do you need the png itself or is it generated from the .json file? I created a similar figure but it doesn't look exactly the same as all the other heatmap mockups.

plotly_cividis

@etpinard
Copy link
Contributor

etpinard commented Nov 23, 2017

Do you need the png itself

Yep, you'll need to the generate the png baseline (see docs) in order to get the tests to pass.

@jamienunez
Copy link
Contributor Author

It looks like I'm unable to do this from my work computer since docker isn't too friendly with Windows 7. Is this an image that can be generated by your team? I can also try using the Linux VM set up on my home computer tonight if that is the easier route.

@alexcjohnson
Copy link
Collaborator

@jamienunez right, the docker setup is a bit finicky even on *nix systems. Anyway I've fixed the syntax and added the baseline image, will merge when tests pass. Thanks for adding cividis!

@alexcjohnson alexcjohnson merged commit b47e174 into plotly:master Nov 27, 2017
@chriddyp
Copy link
Member

Hey @jamienunez - I included this feature in our release notes blog post: https://medium.com/@plotlygraphs/notes-from-the-latest-plotly-js-release-b035a5b43e21. Let me know if you'd like anything changed (chris[at]plot.ly) and thanks again for your contribution!

@jamienunez
Copy link
Contributor Author

This looks great! Thank you for featuring cividis. I really like the animation you used to feature it.

If it isn't too much work, can you change "Community member Jamie Nuñez" to "Community members Jamie Nuñez, Dr. Ryan Renslow, and Dr. Chris Anderton"? We all contributed to this work so it would be nice to see them named as well.

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.

4 participants