Add `from_pandas` and `from_xarray` #2054

Tom-Willemsen · 2021-07-14T12:43:05Z

Fixes #2049

I've implemented attr handling where it makes sense and is supported by both source and target
I haven't implemented any unit handling as it seems to be done in attributes for xarray, and parsing strings felt like it may be a bit too error-prone. Happy to reconsider if there are strong opinions about this.

Tom-Willemsen · 2021-07-14T12:44:59Z

python/src/scipp/compat/xarray_compat.py

+    for attrib in xarray_dataset.attrs:
+        attrib_name = "{}{}".format(attrib_prefix, attrib)
+        if attrib_name in sc_data:
+            raise ValueError(
+                f"Attribute {attrib} would collide with an existing "
+                f"data object '{attrib_name}' (using attrib_prefix "
+                f"'{attrib_prefix}'). Specify a different attrib_prefix "
+                f"in the call to from_xarray_dataset")
+
+        sc_data[attrib_name] = scipp.scalar(xarray_dataset.attrs[attrib])


I'm not sure whether this is the correct approach in scipp or not, scipp.DataSet doesn't seem to support attrs directly unlike scipp.DataArray? The alternative would probably be to drop the xarray attrs (possibly with a warning about loss of information?).

I think dropping is likely the correct approach, especially given that xarray coords (which are also xr.DataArray) can also have attributes, and we don't have a way of supporting that either.

SimonHeybrock

Good start!

I haven't implemented any unit handling as it seems to be done in attributes for xarray, and parsing strings felt like it may be a bit too error-prone. Happy to reconsider if there are strong opinions about this.

If you refer to parsing unit strings, that is supported already (using another library under the hood), you can, e.g., sc.Unit('m/s'). The question is maybe how to find attributes that represent a unit? xarray appears to be commonly used with NetCDF and it looks like the corresponding attribute name is 'units'. Thus I might suggest looking for this attribute and use it as the unit. We may also consider supporting singular 'unit'.

Another things to consider for from_xarray: xarray distinguishes "index coords" (marked with *) and other coords. In terms of how they behave we should:

Convert "index coords" into scipp coords
Convert other coord into scipp attrs

See below for other comments:

SimonHeybrock · 2021-07-15T04:40:36Z

python/src/scipp/compat/pandas_compat.py

@@ -0,0 +1,47 @@
+import pandas
+
+import scipp


Since this is part of scipp cannot import scipp (without running into recursion). Rather use things such as:

from .. import scalar

SimonHeybrock · 2021-07-15T04:42:24Z

python/src/scipp/compat/pandas_compat.py

@@ -0,0 +1,47 @@
+import pandas


If we want to import this file (or functions from it) in __init__.py we have to make this import optional, for example import only when from_pandas is called. Otherwise this turns into a mandatory scipp dependency (module won't load unless pandas is installed).

SimonHeybrock · 2021-07-15T04:50:33Z

python/src/scipp/compat/pandas_compat.py

+        sc_attribs[attr] = scipp.scalar(dataframe.attrs[attr])
+
+    row_index = dataframe.axes[0]
+    rows_index_name = row_index.name or "pandas_row"


Maybe just 'row'?

SimonHeybrock · 2021-07-15T04:55:04Z

python/src/scipp/compat/pandas_compat.py

+        )
+    }
+
+    if dataframe.ndim == 2:


I don't think what is done below will work in general: What pandas calls "2 dimensions" for a DataFrame is actually a 1-dimensional dataset. I think what we should do is to return a 1-D Dataset. dataframe.axes[1] basically gives the names of the dataset entries.

One might consider returning a DataArray if there is only a single column, but maybe that is too surprising and should be avoided.

SimonHeybrock · 2021-07-15T04:58:06Z

python/src/scipp/compat/pandas_compat.py

+        )
+
+    return scipp.DataArray(
+        data=scipp.Variable(values=dataframe.to_numpy(), dims=sc_dims),


The problem here is that to_numpy does type conversions of columns if column dtype mismatches. Even worse, if there are things such as string columns the output array has object dtype (which is inefficient and not well supported by scipp). Basically: See above, return 1-D Dataset instead.

SimonHeybrock · 2021-07-15T05:07:51Z

python/src/scipp/compat/xarray_compat.py

+    for data_key in xarray_dataset.data_vars:
+        sc_data[data_key] = from_xarray_dataarray(
+            xarray_dataset.data_vars[data_key])


Suggested change

for data_key in xarray_dataset.data_vars:

sc_data[data_key] = from_xarray_dataarray(

xarray_dataset.data_vars[data_key])

items = { key:from_xarray(var) for key, var in ds.data_vars.items() }

would be briefer

SimonHeybrock · 2021-07-15T05:13:30Z

python/src/scipp/compat/xarray_compat.py

+        sc_coords[coord] = scipp.Variable(
+            dims=[coord],
+            values=xarray_dataset.coords[coord].values,
+        )


This (make variable from xarray object) is repeated a couple of times, also in from_xarray_dataarray above. Can we use common code? xarray also has a Variable type, but I am not sure that is what you get from xarray_dataset.coords[coord] (that might be a DataArray), so I don't know if you can automatically do this using from_xarray, or need a helper such as _from_xarray_variable:

coords = { key:_from_xarray_variable(var) for key, var in ds.coords.items() }

(ideally we'd want to simply use coords = { key:from_xarray(var) for key, var in ds.coords.items() }, but you will have to see if you can figure out automatically that it should create a Variable and not a DataArray).

SimonHeybrock · 2021-07-15T05:14:47Z

python/src/scipp/compat/xarray_compat.py

+    for attrib in xarray_dataset.attrs:
+        attrib_name = "{}{}".format(attrib_prefix, attrib)
+        if attrib_name in sc_data:
+            raise ValueError(
+                f"Attribute {attrib} would collide with an existing "
+                f"data object '{attrib_name}' (using attrib_prefix "
+                f"'{attrib_prefix}'). Specify a different attrib_prefix "
+                f"in the call to from_xarray_dataset")
+
+        sc_data[attrib_name] = scipp.scalar(xarray_dataset.attrs[attrib])


I think dropping is likely the correct approach, especially given that xarray coords (which are also xr.DataArray) can also have attributes, and we don't have a way of supporting that either.

SimonHeybrock · 2021-07-15T05:16:18Z

python/tests/compat/pandas_compat_test.py

+    assert (sc_dataarray.values == scipp.array(dims=["pandas_row"],
+                                               values=[1, 2, 3]).values).all()


Use

Suggested change

assert (sc_dataarray.values == scipp.array(dims=["pandas_row"],

values=[1, 2, 3]).values).all()

assert sc.identical(sc_dataarray, scipp.array(dims=["pandas_row"], values=[1, 2, 3]))

This will also ensure that unit, dimension labels, coords, ... are correct. Same goes for everything below, you should basically never assert using the values or value property.

SimonHeybrock · 2021-07-15T05:18:18Z

python/tests/compat/xarray_compat_test.py

+from scipp.compat.xarray_compat import from_xarray
+
+
+def test_empty_attrs_dataarray():


Can't see a test with coords?

…han a single 2D DataArray

…t support attrs

SimonHeybrock · 2021-07-16T05:05:08Z

python/src/scipp/compat/pandas_compat.py

+                                         coords={
+                                             rows_index_name:
+                                             Variable(dims=[rows_index_name],
+                                                      values=row_index)
+                                         },


Isn't this the same coord that gets added below? No need to repeat it here?

SimonHeybrock · 2021-07-16T05:05:43Z

python/src/scipp/compat/pandas_compat.py

+                                         },
+                                         name=column_name or "")
+
+    return Dataset(data=sc_data,


If we get a Series, should we return a DataArray (or even just a Variable)?

I've modified the code so that if a Series is passed in, a DataArray is returned rather than a Dataset. However for the 1-D Dataframe case it is still returning a Dataset (with 1 item) rather than DataArray because I felt that changing the return type based on the number of dimensions (rather than the type of object being used) would be too confusing as an API.

SimonHeybrock · 2021-07-16T05:07:38Z

python/src/scipp/compat/xarray_compat.py

+    """
+    Converts xarray coords to scipp coords.
+
+    Note: only converts dimension coords


Code doesn't look like that?

SimonHeybrock · 2021-07-16T05:11:14Z

python/src/scipp/compat/xarray_compat.py

+    coords = {}
+
+    for coord_name, coord in xr_obj.coords.items():
+        is_index = (coord_name in xr_obj.indexes)
+        if is_index == index:
+            coords[coord_name] = Variable(dims=coord.dims, values=coord.values)
+    return coords


How about:

coords = {} attrs = {} for name, coord in xr_obj.coords.items(): if name in xr_obj.indexes: coords[name] = else attrs[name] = _var_from_xarray(coord) return coords, attrs

with

def _var_from_xarray(xr_obj): unit = coord.attrs['units'] if units in coord.attrs else '' return Variable(dims=coord.dims, values=coord.values, unit=unit)

The latter should be used everywhere, since we want to look for units everywhere.

SimonHeybrock · 2021-07-16T05:14:47Z

python/src/scipp/compat/xarray_compat.py

+    :param ds: an xarray.Dataset object to be converted.
+    :return: the converted scipp dataset object.
+    """
+    sc_data = {k: from_xarray(v) for k, v in ds.data_vars.items()}


Why is the ds.data_vars.items() needed? What is the difference to ds.items()?

SimonHeybrock · 2021-07-19T04:45:06Z

python/src/scipp/compat/xarray_compat.py

+        if name in da.indexes:
+            coords[name] = _var_from_xarray(coord)
+        else:
+            attrs[f"coord_{name}"] = _var_from_xarray(coord)


I don't think we should be renaming the coords? I suppose you do this to avoid clashes with xarray-attributes? I think we should either rename the xarray-attributes, or simply drop them if there is a name clash.

SimonHeybrock

Edit: Sorry, had posted comment as individual comment above, ignore this.

SimonHeybrock · 2021-07-22T06:36:28Z

Added link to these new features in docs and release notes as part of #2058.

Tom-Willemsen added 5 commits July 13, 2021 16:31

Add initial converter & tests from xarray.DataArray

4c16ceb

Add initial converter for xarray.Dataset

7267ad1

Add initial converter from pandas.DataFrame and pandas.Series

2d6e6ae

Code cleanup

db99a1e

Add method docs

20f436b

Tom-Willemsen commented Jul 14, 2021

View reviewed changes

Add xarray and pandas to test dependencies

3f1dbc9

SimonHeybrock changed the title ~~Issue2049 add xarray pandas converters~~ Add from_pandas and from_xarray Jul 15, 2021

SimonHeybrock requested changes Jul 15, 2021

View reviewed changes

Tom-Willemsen added 7 commits July 15, 2021 11:00

Refactor tests & implement review suggestions

a79053e

Clean up code

1f3fc24

Return Dataset containing multiple 1D DataArrays for pandas, rather t…

fad8033

…han a single 2D DataArray

Remove type hint in test methods

e2f8a19

Handle non-indexed coords from xarray

326e299

Remove pandas attr test as we are now returning dataset which does no…

a5ff8b9

…t support attrs

Use specific dtype

466d0c0

SimonHeybrock reviewed Jul 16, 2021

View reviewed changes

Tom-Willemsen added 3 commits July 16, 2021 10:37

Support units in conversions from xarray. Minor code cleanups.

964cfd7

For pandas series objects return DataArray rather than Dataset

6f84bb2

For pandas series objects return DataArray rather than Dataset

9a53ce9

SimonHeybrock reviewed Jul 19, 2021

View reviewed changes

Don't rename coords

a57cc02

SimonHeybrock approved these changes Jul 19, 2021

View reviewed changes

SimonHeybrock merged commit 425de99 into main Jul 19, 2021

SimonHeybrock deleted the Issue2049_add_xarray_pandas_converters branch July 19, 2021 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `from_pandas` and `from_xarray` #2054

Add `from_pandas` and `from_xarray` #2054

Tom-Willemsen commented Jul 14, 2021 •

edited by SimonHeybrock

Tom-Willemsen Jul 14, 2021

SimonHeybrock Jul 15, 2021

SimonHeybrock left a comment

SimonHeybrock Jul 15, 2021

SimonHeybrock Jul 15, 2021

SimonHeybrock Jul 15, 2021

SimonHeybrock Jul 15, 2021

SimonHeybrock Jul 15, 2021

SimonHeybrock Jul 15, 2021

SimonHeybrock Jul 15, 2021

SimonHeybrock Jul 15, 2021

SimonHeybrock Jul 15, 2021

SimonHeybrock Jul 15, 2021

SimonHeybrock Jul 16, 2021

SimonHeybrock Jul 16, 2021

Tom-Willemsen Jul 16, 2021

SimonHeybrock Jul 16, 2021

SimonHeybrock Jul 16, 2021

SimonHeybrock Jul 16, 2021

SimonHeybrock Jul 16, 2021

SimonHeybrock Jul 19, 2021

SimonHeybrock left a comment •

edited

SimonHeybrock commented Jul 22, 2021

		assert (sc_dataarray.values == scipp.array(dims=["pandas_row"],
		values=[1, 2, 3]).values).all()

	assert (sc_dataarray.values == scipp.array(dims=["pandas_row"],
	values=[1, 2, 3]).values).all()
	assert sc.identical(sc_dataarray, scipp.array(dims=["pandas_row"], values=[1, 2, 3]))

		from scipp.compat.xarray_compat import from_xarray


		def test_empty_attrs_dataarray():

Add from_pandas and from_xarray #2054

Add from_pandas and from_xarray #2054

Conversation

Tom-Willemsen commented Jul 14, 2021 • edited by SimonHeybrock

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonHeybrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimonHeybrock left a comment • edited

Choose a reason for hiding this comment

SimonHeybrock commented Jul 22, 2021

Add `from_pandas` and `from_xarray` #2054

Add `from_pandas` and `from_xarray` #2054

Tom-Willemsen commented Jul 14, 2021 •

edited by SimonHeybrock

SimonHeybrock left a comment •

edited