1141 | Validate canvas width, height

[Jap8nted]

See issue #1141.

When a canvas is created with plot_width=0 or plot_height=0 and the data is coming from dask, there is a core dump error, up to now, the error could just be reproduced on a machines running amd and intel. On Arm the problem does not appear.

from datashader import Canvas
from datashader.reductions import mean, sum, max, min
from dask import dataframe as ddf
def main():
    df = pd.DataFrame({"LON": [0, 1, 2, 3, 4], "LAT": [0, 1, 2, 3, 4], "VALUE": [0, 1, 2, 3, 4]})
    data = ddf.from_pandas(df, npartitions=1)
    cvs = Canvas(
        plot_width=0,
        plot_height=0
    )
    cvs.points(data, "LON", "LAT", agg=max("VALUE"))
if __name__ == "__main__":
    main()

[jbednar]

Thanks. I'll leave it to @ianthomas23 to think about whether it's appropriate to raise an error or to return an empty array in this case; not sure which is e.g. less problematic for HoloViews.

[ianthomas23]

OK, so the options here are

1. Raise an error.
2. Continue as normal and return zero-sized DataArrays.

I'd be happy with either but my preference is for 1 as I'd rather detect the situation as soon as possible and report it immediately.

@Jap8nted Are you happy to continue with this PR?

If so, this fix is good, it is failing CI because the raised ValueError doesn't need to use an f-string. Also, it should have a test to confirm that the fix is good. A new test in test_pandas.py should suffice, perhaps just below test_line_coordinate_lengths and it can use similar pytest.raises logic to that test.

[Jap8nted]

Hi,

I will continue with it, @ianthomas23.

In my opinion, raising an error is better as we skip unnecessary calculations.

will fix it later and check that the CI finishes successfully.

[codecov]

Codecov Report

Merging #1183 (51e7f18) into main (198c0b4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 51e7f18 differs from pull request most recent head 58178ca. Consider uploading reports for the commit 58178ca to get more accurate results

@@           Coverage Diff           @@
##             main    #1183   +/-   ##
=======================================
  Coverage   85.38%   85.39%           
=======================================
  Files          35       35           
  Lines        8011     8016    +5     
=======================================
+ Hits         6840     6845    +5     
  Misses       1171     1171           

Impacted Files	Coverage Δ
datashader/core.py	88.35% <100.00%> (+0.09%)	⬆️
datashader/transfer_functions/__init__.py	86.91% <0.00%> (+0.02%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[ianthomas23]

@Jap8nted This is functionally fine. I've manually tested it using cudf and cudf-dask as our CI doesn't do this for us and they both fail correctly.

I would rather call it validate_size than validate_extend so I have made a few suggested changes along those lines, and also a few changes to whitespace. If you could make those amendments it should be good to merge.

[ianthomas23]

Thanks @Jap8nted.

[Jap8nted]

Thanks for the comments and help @ianthomas23 and happy to help!