holoviews icon indicating copy to clipboard operation
holoviews copied to clipboard

Setting expand to false for Image

Open hoxbro opened this issue 1 year ago • 5 comments

Fixes https://github.com/holoviz/holoviews/issues/5358

To be honest, this fix is more of a workaround for the problem than a fix. But the complexity of the the holoviews pipelines makes it hard for me to see when self.p.expand is set to True, even though regrid.expand defaults to False. Suggestions are welcome.

With this change the notebook example outputs this:

https://user-images.githubusercontent.com/19758978/177816756-b6749b29-282f-4097-a1ae-daf4a7eed9a2.mp4

The example in the notebook can be reduced to this MRE:

import xarray as xr
import numpy as np
import holoviews as hv
from holoviews.operation.datashader import rasterize

da = xr.DataArray(
    np.arange(10_000).reshape(100, 100),
    coords=dict(
        x=np.linspace(166021, 534994, 100),
        y=np.linspace(0.00, 9329005, 100),
    ),
)
rasterize(hv.Image(da))

Before this fix:

https://user-images.githubusercontent.com/19758978/177817920-cb102b31-8887-4b1f-9e78-cfe77f92c334.mp4

After this fix:

https://user-images.githubusercontent.com/19758978/177817400-b2837463-019d-4ead-90cd-cd54c840e7a9.mp4

hoxbro avatar Jul 07 '22 15:07 hoxbro

Codecov Report

Merging #5351 (ac300a4) into master (5ce1e1d) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #5351   +/-   ##
=======================================
  Coverage   88.07%   88.07%           
=======================================
  Files         301      301           
  Lines       61957    61959    +2     
=======================================
+ Hits        54567    54569    +2     
  Misses       7390     7390           
Impacted Files Coverage Δ
holoviews/operation/datashader.py 84.01% <100.00%> (+0.02%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5ce1e1d...ac300a4. Read the comment docs.

codecov-commenter avatar Jul 07 '22 16:07 codecov-commenter

Hmm. It's good that this fixes the symptom, but I agree it seems like a workaround, because expand shouldn't affect whether the plot works ok; it should only affect whether the image is regridded into the entire viewport or only into the original extent of that image. Both options should result in the same visual display for the default transparency and other settings, so if the image renders only for one of the settings, I think that's a bug rather than an indication that we should use that setting.

jbednar avatar Jul 07 '22 20:07 jbednar

This workaround works because it avoids setting x_range = self.p.range in the following lines but instead calculates them based on the element range. Where self.p.range is the plot's range, which changed when moving around the plot.

https://github.com/holoviz/holoviews/blob/2d2b72809eb2e2d75f1e26f3fad97892b2d6a720/holoviews/operation/datashader.py#L145-L154

Same thing with y_range.

I tried always calculating the range, but this made the tests fail.

hoxbro avatar Jul 08 '22 07:07 hoxbro

I've discussed this with @Hoxbro and I see a number of overlapping issues:

  1. It is possible I am forgetting something, but where does the fill value come from when expanding? This should be settable but I don't see where to do that right now (if so, the docstrings need improving!). Of course, for images you can control the visual display at the colormapping level but you should still have this control over the data generated...
  2. You can't specify the transparent value when color mapping for RGBs as RGBs aren't color mapped. I would argue that regrid (or rasterize in hvplot) for RGBs should use white+transparent (0% alpha) in the region where there is no data. I suppose this could also be exposed as an RGB specific option where you specify an RGB or RGBA tuple...
  3. Is expand an option that should be exposed at the hvplot level and I'm just wondering if the default of expand=True was an explicit decision with a good rationale?

Once we are clear on these issues, then it should become more obvious what the correct fix is...

Edit: I'm remembering a bit more now, I believe that the black for NaNs comes back to an old convention in datashader, in which case matching that for missing values made some sense...but transparent pixels are an equally valid and useful choice! Do you agree @jbednar ?

jlstevens avatar Jul 13 '22 10:07 jlstevens

I agree. I don't remember all the details, but would have thought we always preferred transparent for NaN, albeit with black as the color, which wouldn't normally be visible.

jbednar avatar Jul 13 '22 18:07 jbednar

Superseded by https://github.com/holoviz/holoviews/pull/5767

hoxbro avatar Jun 20 '23 10:06 hoxbro