Closed jonathansick closed 9 years ago
@jiffyclub Looks like there are two test issues here:
OrderedDict
there. I can just go back to using lists; or at least keep a copy of the colormap names in a separate ordered list.You can see in palette.py how I'm protecting matplotlib imports. There's enough numpy usage in there that I don't necessarily want to make you replace it all with pure python. What if the data for all the default palettes was pre-computed and stored in the file so that people only needed numpy when creating custom versions of cubehelix? It looks like the default palettes are defined by 256 RGB points, which is probably more than strictly necessary. matplotlib does linear interpolation between the points you give it, so if you generated even 16 RGB points for the default palettes I bet they'd look decent (though that'd require some testing).
I can take care of adding a doc page before the next release, but do feel free to add a notebook to the demo/ directory.
The colors as 2D array thing works because ndarrays are sufficiently list like when you iterate over them. You get a 1D row of length three on each iteration, which is what the code is expecting. All the looping looks like for color in self.colors
.
We'll need to fix the hex colors coming out the the L's, that's not right. There must be long integers:
In [6]: '{:>02}'.format(hex(128L)[2:].upper())
Out[6]: '80L'
We can fix that by coercing to int
before converting to hex here.
Here's an example of how you don't need a large number of RGB points to make these palettes:
So I think we could pre-generate the default cubehelix palettes and store the numbers in there so folks can access them with a pure Python install of palettable. Maybe add a class method or something for creating a Cubehelix
instance from pre-generated numbers?
Yeah! I'll push something soon with the class method and pre-baked RGB lists. I'm just at a meeting right now at github of all places.
Looks great! I saw you went with a class method for making the custom palettes. That was the only way I could think to make it work as well. An alternative solution would be to keep the Cubehelix class as you had it originally but have the default ones be Palette instances instead. But then they aren't Cubehelix instances. I'm happy with the Cubehelix.make class method if you are.
I think the class method makes total sense. Better to keep the API uniform and be explicit about generating the palette rather than using one
Do you have any stylistic requests? I'll fix the test coverage gaps.
Looks great! Such PEP8, very wow. :smile_cat:
I think the last thing to make sure of is that we aren't ending up with L's in the hex color strings.
I think I've resolved the last few details:
rgb_array.astype(int).tolist()
n
argument from palettable.cubehlix.get_map
since it doesn't make any sense here. See 542a165 for how that's handled.One thing I just remembered is that I've started putting the number of defined colors at the end of the palette names. So e.g. in the tableau module there are palettes named like BlueRed_6 and BlueRed_12. Here those would all be _16, but for consistency I think it should be there. I put that suffix there so people accessing the palettes via tab completion can quickly tell how many defined colors there are.
Looking good! Ready to go?
@jiffyclub I think so! Do you want me to squash everything into one commit?
Also, I was trying to make the docs with cubehelix but urubu was getting links wrong (even to the existing sub pages). So maybe I'll leave docs off of this PR after all.
Urubu can make it hard to preview things locally because they have to served from a http://host/palettable/ URL. I had to make the tservice tool specifically for previewing these docs locally, e.g. with tserve --prefix palettable _build
.
I don't care one way or the other about squashing commits. If you want to do any cleanup go for it, but I'm happy to merge whenever.
Okay, give me a sec and I'll see if I can ship some docs with it.
I've put together a doc page for cubehelix. It includes both previews and an API reference section for making palettes. I don't entirely like just listing the arguments the way I did, but it works. Maybe laying out the API via a table would be better.
:shipit:?
A table would work, but I don't want to muck around with a table in Markdown. Honestly I was just going to put in the raw output of print(Cubehelix.__doc__)
, so :+1:. Merging!
Thanks for your help with this!
Thank you!
This PR implements cubehelix color maps in palettable by porting @jradavenport's
cubehelix.py
. I put a demo notebook up as a gist here. I think it's sweet.I've kept the API for
palettable.cubehelix
as similar as possible to the other palettes. Basically, theCubehelix(Palette)
class computes colors for the color map.get_map()
is a factory that takes pre-defined parameter sets and createsCubehelix
instances. The user is also encouraged to build their own maps directly.I've spruced up the cubehelix algorithm implementation a bit. Some parameters are renamed for clarity. I also use
numpy.dot()
to build the entire RGB color vector in one statement. Anyways, it's a bit more Pythonic and a little less... Fortranic?I also noticed a discrepancy between D.A. Green's formula for
phi
and the Fortran code (and the original Python implementation); there's an issue of adding an extra+1
. I've stayed true to the formula in the paper, although I don't actually notice any difference in the outputs.Some issues I see:
palettable.cubehelix
.(n, 3)
numpy array rather than a list of 3-tuples. Surprisingly, this works. The one problem is that hex colors contain an'L'
, e.g.,'#23L74L33L'
. What's up with this? Along
type? Is this ok?demo/
if you think that's a good idea.Let me know what you think!