jupyter-widgets / ipyleaflet

A Jupyter - Leaflet.js bridge
https://ipyleaflet.readthedocs.io
MIT License
1.49k stars 364 forks source link

Remove xarray dependency #212

Closed jasongrout closed 5 years ago

jasongrout commented 6 years ago

xarray is a rather heavy dependency for ipyleaflet, and as far as I can tell is not strictly required (it's not imported anywhere I can see). Do we need it as a dependency? Can we remove it as a dependency?

SylvainCorlay commented 6 years ago

xarray is a dependency because we use it for the Dataset attribute of the velocity layer.

(It is an optional dependenxy of traittypes required to use the dataset).

jasongrout commented 6 years ago

Looking more closely, it seems that the xarray python package is not too bad, but the conda-forge package pulls in a lot of heavier dependencies: https://github.com/conda-forge/xarray-feedstock/blob/acb13afdc2dbb9f1c8c084a03392b23f28b22b6c/recipe/meta.yaml#L20-L28

SylvainCorlay commented 6 years ago

???

    - dask >=0.9
jasongrout commented 6 years ago

and netcdf and h5netcdf

ocefpaf commented 6 years ago

Looks like xarray dataset is used to fill this dictionary:

        velocity_data.append({
            "header": {
                "parameterUnit": units,
                "parameterNumber": p_number,
                "dx": dx, "dy": dy,
                "parameterNumberName": p_name,
                "la1": lat_upper,
                "la2": lat_lower,
                "parameterCategory": 2,
                "lo2": lon_right,
                "nx": nx,
                "ny": ny,
                "refTime": "2017-02-01 23:00:00",
                "lo1": lon_left
                },
            "data": ds[var_name].values.flatten().tolist()
        })

I would argue that ipyleaflet should be more generic, so people can fill those info with other tools (numpy array, iris, lists, etc), and drop the xarray dependency. I understand that a high level object makes it easier for coding something like that but it could be wrapped by users outside of ipyleaflet and they can adapt to any other dataset/metadata model they have.

SylvainCorlay commented 6 years ago

Eventually we will be using ipywidgets binary serializarion for the data.

Velocity data is generally large homogeneous arrays so it should at least be numpy instead of JSON.

Now xarray makes it easier to not show things upside down etc. There are many ways to get things wrong with all the dimensions.

ocefpaf commented 6 years ago

Now xarray makes it easier to not show things upside down etc.

I get that but I would still argue that an array-like object is better. ipyleaflet can still provide an xarray convenience function and make xarray an optional dependency (that is my plan for the folium version of that plugin BTW).

rabernat commented 6 years ago

Just found this issue and adding my $0.02. (Disclaimer: I am an xarray dev.)

There could be advantages if xarray would become a standard data structure for labeled ndarrays, especially among geospatial packages. It may save other packages a lot of boilerplate code, since they don't have to re-solve the same problems that xarray has already solved. For example:

There are many ways to get things wrong with all the dimensions.

👍... ipyleaflet is a perfect example of such a package.

One possible solution is to not eliminate the xarray dependency here, but rather to make the xarray conda-forge dependencies lighter.

jasongrout commented 6 years ago

One possible solution is to not eliminate the xarray dependency here, but rather to make the xarray conda-forge dependencies lighter.

That's the solution in https://github.com/conda-forge/xarray-feedstock/pull/37

jhamman commented 5 years ago

@jasongrout - the current xarray build on conda-forge is much lighter now. Should fix the problems listed above.

SylvainCorlay commented 5 years ago

Thanks. Closing for now as xarray has become more lightweight on conda.

We may want to open another issue on allowing ordinary numpy arrays

scottyhq commented 5 years ago

@jasongrout and @SylvainCorlay , shouldn't the following line in setup.py be removed or changed to allow for more recent xarray versions (currently at 0.11.3)

https://github.com/jupyter-widgets/ipyleaflet/blob/15f33691edf07438bfa9534f9c6949d529119632/setup.py#L139

SylvainCorlay commented 5 years ago

@scottyhq indeed we probably need to support 0.11 as well. I think this deserves its own issue.