plotly.py icon indicating copy to clipboard operation
plotly.py copied to clipboard

add colormodel parameter into px.imshow and test

Open liudj2008 opened this issue 5 years ago • 9 comments

  • [x] I have read through the contributing notes and understand the structure of the package. In particular, if my PR modifies code of plotly.graph_objects, my modifications concern the codegen files and not generated files.
  • [x] I have added tests (if submitting a new feature or correcting a bug) or modified existing tests.
  • [ ] For a new feature, I have added documentation examples in an existing or new tutorial notebook (please see the doc checklist as well).
  • [ ] I have added a CHANGELOG entry if fixing/changing/adding anything substantial.
  • [x] For a new feature or a change in behaviour, I have updated the relevant docstrings in the code to describe the feature or behaviour (please see the doc checklist as well).

liudj2008 avatar Nov 24 '20 14:11 liudj2008

Thanks a lot for the PR @liudj2008 ! The CI failures probably don't have anything to do with your PR: on py2.7, it looks like a new version of qtconsole forgot to say they were not compatible with python 2.7, but they seem to have fixed this so I restarted the build :crossed_fingers: (@nicolaskruchten this might affect some other PRs). Still investigating the doc problem.

emmanuelle avatar Nov 26 '20 16:11 emmanuelle

One difference between RGB and HSL is that the bounds for the channels are different:

  • for RGB the 3 channels are expected to be between 0 and 255
  • for HSL, H is in [0, 360] (an angle), S in [0, 100] and V in [0, 100]

Therefore I think we should disable binary_string=True when colormodel='hsl'/'hsla' since binary string implies values in [0. 255]. Also, if zmax is None it should be set to [360, 100, 100]. Could you please make these changes as well?

Here is an example (using the colour-science package) which might be worth adding to the doc: (doc/python/imshow.md, I can also do it in a subsequent PR)

from skimage import data, color, img_as_ubyte, img_as_float
import plotly.express as px
img = data.chelsea()
import colour
img_hsl = colour.RGB_to_HSL(img_as_float(img))
img_hsl_360 = (img_hsl * np.array([360, 100, 100])[np.newaxis, np.newaxis, :]).astype(np.uint16)
fig = px.imshow(img_hsl_360, binary_string=False, colormodel='hsl', zmax=[360, 100, 100])
fig.show()

emmanuelle avatar Nov 26 '20 21:11 emmanuelle

For the build-doc CI issue, could you please update your master branch, and then merge master into your feature branch to see if this solves the problem?

emmanuelle avatar Nov 27 '20 12:11 emmanuelle

Therefore I think we should disable binary_string=True when colormodel='hsl'/'hsla' since binary string implies values in [0. 255]

I'm not sure why this would mean we can't use binary_string? We can use HSL input to create a PNG, no?

nicolaskruchten avatar Nov 27 '20 13:11 nicolaskruchten

@nicolaskruchten we could indeed take the HSL input and convert it back to rgb to make a png, but then we're not making use of the HSL capability of the Image trace which was the original idea. With an H channel between 0 and 260 it's not possible to encode it on one byte. In fact I have no idea whether there is a native format for HSL images...

emmanuelle avatar Nov 27 '20 13:11 emmanuelle

but then we're not making use of the HSL capability of the Image trace which was the original idea

Not sure I fully agree here... to me the original idea is to allow users to provide input data in HSL format, and ideally we would display that in the fastest way possible, no, regardless of the details of the underlying JS implementation? Maybe Plotly.js still needs to be told "hey this is meant to be HSL" so it can do the hover in HSL mode (essentially picking up the RGB value of the PNG pixel and reversing the HSL-to-RGB conversion) ?

nicolaskruchten avatar Nov 27 '20 13:11 nicolaskruchten

I agree with @nicolaskruchten , users do not have to worry about implementation, as long as we can "recognize" hsl from rga and show it properly. I do notice in graph_objs/_image.py, when initiating Image class, all we need to do is to pass in z and colormodel, and zmin and zmax will be defaulted depending on colormodel input (so we do not have to worry about the outrange issue). As @emmanuelle pointed out, the definition of binary_string in px module is to "force" image conversion to rgb/rgba so that to construct the img source to pass in go.Image, and we need to disable it for hsl/hsla images.

So in that case, I will call future px on hsl image like this (no zmax, since it will be defaulted): fig = px.imshow(img_hsl_360, binary_string=False, colormodel='hsl')

liudj2008 avatar Nov 27 '20 15:11 liudj2008

@emmanuelle So I merged the updated master with the branch, but still had the build-doc issue. Seems like it is related with notebook to html conversion. Is there anything I can do to fix it? I will add the modification to doc/python/imshow.md once this is fixed. Thanks!

liudj2008 avatar Nov 27 '20 18:11 liudj2008

@archmoj can you please have a look and decide (a) if this is still relevant and (b) if the hard-coded 3's and 4's can now be replaced by symbolic constants? thanks - @gvwilson

gvwilson avatar Aug 23 '24 14:08 gvwilson