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

New Perceptual Colormaps #204

Merged
merged 8 commits into from
Aug 8, 2014
Merged

New Perceptual Colormaps #204

merged 8 commits into from
Aug 8, 2014

Conversation

rkern
Copy link
Member

@rkern rkern commented Jun 18, 2014

I have added a few perceptually-based colormaps from various sources.



# XYZ white-point coordinates
# from http://www.aim-dtp.net/aim/technology/cie_xyz/cie_xyz.htm
Copy link
Member

Choose a reason for hiding this comment

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

This is an ad site now. The referenced page is in the web archive.
https://web.archive.org/web/20080705052651/http://www.aim-dtp.net/aim/technology/cie_xyz/cie_xyz.htm

@cfarrow
Copy link
Member

cfarrow commented Jul 30, 2014

The content is good and worth keeping. However, I don't think chaco is the best home for the supporting code. This code is widely usable, vetted at some level since it came from scipy, and should probably be made into its own package. Some of this is already in scikit-image.

I don't necessarily want chaco to depend on scikit image, but I'd also like to avoid taking on lots of new specialized code.

@rkern
Copy link
Member Author

rkern commented Jul 30, 2014

I can pare down the colorspace stuff that isn't used, but I'm opposed to adding a dependency.

@enbuilder
Copy link

Merged build triggered.

@enbuilder
Copy link

Merged build started.

@rkern
Copy link
Member Author

rkern commented Jul 30, 2014

Alternately, I can add my colorblindness-simulating meta-ColorMappers that use the rest of the colorspace functions. ;-)

@coveralls
Copy link

Coverage Status

Coverage decreased (-10.51%) when pulling e22f7a7 on feature/new-colormaps into 032f21a on master.

@cfarrow
Copy link
Member

cfarrow commented Jul 30, 2014

Adding new colormaps defeats the YAGNI argument for the supporting code. (I don't think YAGNI applies to colormaps until they become visually indistinct.) +1

Dependence on scikit-image is off the table. If creating a new package (scikit-color?) is out of the question, then we need to test the new colormaps. I'm less worried about correctness than code breaking with numpy 1.9 or python 3 (eventually). Some high level tests that instantiate the new colormaps and verifies any meaningful metrics should suffice.

@enbuilder
Copy link

Merged build finished.

@jonathanrocher
Copy link
Collaborator

@rkern please also note the drastic decrease in test coverage.

@rkern
Copy link
Member Author

rkern commented Jul 30, 2014

Noted, but it appears unrelated to the changes here.

@jonathanrocher
Copy link
Collaborator

Yes we have had that +/-10% oscillation for a while. I need to find some time to look into this...

@enbuilder
Copy link

Merged build triggered.

@enbuilder
Copy link

Merged build started.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.58%) when pulling 4f7d3f4 on feature/new-colormaps into 032f21a on master.

@enbuilder
Copy link

Merged build finished.

@cfarrow
Copy link
Member

cfarrow commented Aug 4, 2014

LGTM. Thanks.

rkern added a commit that referenced this pull request Aug 8, 2014
@rkern rkern merged commit 4faecda into master Aug 8, 2014
@rkern rkern deleted the feature/new-colormaps branch August 8, 2014 09:25
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.

None yet

5 participants