[ENH] support for `xarray` `DataArray` & mtypes by benHeid · Pull Request #3255 · sktime/sktime

benHeid · 2022-08-13T09:45:57Z

Reference Issues/PRs

resolves #2655

What does this implement/fix? Explain your changes.

Does your contribution introduce a new dependency? If yes, which one?

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions

I've added myself to the list of contributors.
Optionally, I've updated sktime's CODEOWNERS to receive notifications about future changes to these files.
I've added unit tests and made sure they pass locally.
The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.

For new estimators

I've added the estimator to the online documentation.
I've updated the existing example notebooks or provided a new one to showcase how my estimator works.

benHeid · 2022-08-13T09:47:48Z

TODOs

make xarray to a softdependency (conditional import)
resolve TODOs in the code
Discuss if
- DataSets should also be supported
- Further scitypes should support xarray

fkiraly · 2022-08-13T10:21:32Z

for conditional execution (based on presence of package in the env), you can use:

if _check_soft_dependencies("xarray", severity="none"):
    import xarray
    etc

fkiraly · 2022-08-14T08:29:47Z

let me know if you think you are stuck - I can look at this is more detail, we can do pair programming, or I can give it a try.

benHeid · 2022-08-15T08:12:38Z

let me know if you think you are stuck - I can look at this is more detail, we can do pair programming, or I can give it a try.

Thank you, I let you know if I am stuck

benHeid · 2022-08-15T08:36:55Z

I have two questions:

I added the extend_conversion function from tables to series. (In panel, there exists something similar). Should I move this method to another file in the datatypes file, or should an extra PR do this?
Should I add xarray as a soft dependency somewhere that the automatic tests can install xarray?

fkiraly · 2022-08-15T08:38:58Z

I added the extend_conversion function from tables to series. (In panel, there exists something similar). Should I move this method to another file in the datatypes file, or should an extra PR do this?

Depends what you need it for. If needed in this PR, we can leave it in its location and refactor.
If you want it as a more general utility and not needed for the xarray topic, it is better putting in a separate PR.

Should I add xarray as a soft dependency somewhere that the automatic tests can install xarray?

Yes, pyproject.toml, all_extras dependency set.

* Use store dict for xarray DataArray conversion * Fix linting errors in newly added code.

benHeid · 2022-08-15T10:38:58Z

I added the extend_conversion function from tables to series. (In panel, there exists something similar). Should I move this method to another file in the datatypes file, or should an extra PR do this?

Depends what you need it for. If needed in this PR, we can leave it in its location and refactor. If you want it as a more general utility and not needed for the xarray topic, it is better putting in a separate PR.

Ok, it is not needed for the xarray topic.

benHeid · 2022-08-15T15:00:04Z

Update:

I am currently working on probably transforming the index from pandas to xarray and vice versa. I assume that this is caused by the transpose which I apply to have the same shape after the conversion chain: pd.DataFrame -> xr.DataArray -> pd.DataFrame.

A solution for this will probably also solve also the problem with the tests in python 3.7 (it seems that in python 3.7 the index is a BaseIndex after the conversion, while for 3.8 and 3.9 it is a PeriodIndex)

…ons which swap the dimensions.. * Append deep_equals for xarray testing.

fkiraly · 2022-08-15T15:43:17Z

A solution for this will probably also solve also the problem with the tests in python 3.7 (it seems that in python 3.7 the index is a BaseIndex after the conversion, while for 3.8 and 3.9 it is a PeriodIndex)

Ouch... how odd. I suppose this is one of those non-obvious problems that occur...

fkiraly · 2022-08-16T07:57:29Z

👏 nice! Looks like it passes now!

sktime/utils/_testing/deep_equals.py

sktime/datatypes/_series/_check.py

benHeid · 2022-08-16T11:18:39Z

sktime/datatypes/_series/_convert.py

+    return concat_fun
+
+
+def _extend_conversions(mtype, anchor_mtype, convert_dict):


If datatypes is refactored, then this method should be placed somewhere else. Currently similar methods are implemented in panels and tables.

yes, indeed - too much copy-paste!
Though I would go with incurring technical debt due to priorities...

Re refactor directions:

@chrisholder started sth, but abandoned it.
#1460
Haven´t had the time to salvage it.

This would be another option:
dylan-profiler/visions#194

sktime/datatypes/_series/_convert.py

benHeid · 2022-08-16T11:22:09Z

👏 nice! Looks like it passes now!

Yeah 🥳
I think you can now take a more detailed look at this PR. I added already some comments, and I would appreciate your feedback there.

Should we add the support for other mtypes and the dataset in this PR or should I create additional PRs for the other?

fkiraly · 2022-08-16T17:01:27Z

Should we add the support for other mtypes and the dataset in this PR or should I create additional PRs for the other?

I would go with the "minimal modular change" principle and separate PR, also finalizing this one first.
Which other mtypes do you have in mind?

fkiraly

Really nice, thanks!

if this were not a draft, I would have only one blocking change, that's the coupling introduced in deep_equals, can we avoid it?

* Enable xarray with only one dimension (Explicit transformation to pandas DataFrame instead of using to_pandas) * Add identity transformation from xr.DataArray to xr.DataArray

benHeid · 2022-08-19T13:30:11Z

Update

I changed the check in deep_equals.
I manually create a dataframe instead of using to_pandas. This ensures that xr.DataArrays are always transformed to DataFrames and not to pd.Series if the xr.DataArray contains only one dimension. Note, to_dataframe does not work well, since it does some multi-indexing stuff in case of two dimension which needs to be unstacked.

sktime/utils/_testing/deep_equals.py

fkiraly

Works!

Do you want me to merge this, or wait for panel?

benHeid · 2022-08-26T06:59:21Z

Works!

Do you want me to merge this, or wait for panel?

I would merge it and add the panel with a second PR if I need this. Currently, I can test and go on with the sktime-pyWATTS integration without implementing Panels. Perhaps Issue #2655 should stay open in that case.

benHeid · 2022-08-26T09:40:09Z

@fkiraly Do we have to adapt utils.validation.series? In that file, something like VALID_DATA_TYPES is defined. Do I have to add xr.DataArray?

fkiraly · 2022-08-26T12:58:09Z

@fkiraly Do we have to adapt utils.validation.series? In that file, something like VALID_DATA_TYPES is defined. Do I have to add xr.DataArray?

I think that's used only in legacy code, so I would recommend to not touch it (who knows what will break).

The "critical touch point" is check_series, which is still linked to a few estimators that have not been fully refactored, such as ColumnwiseTransformer - but mostly decoupled from the vast majority of the code base.

Enable xarray DataArray mtype as sctpye series.

2fc6abe

fkiraly added the module:datatypes datatypes module: data containers, checkers & converters label Aug 14, 2022

Use xarray as soft dependency.

9c12438

Extend the conversion to np.array and pd.Series.

2d8aa9c

* Added xarray to pyproject.toml

f1ce864

* Use store dict for xarray DataArray conversion * Fix linting errors in newly added code.

benHeid changed the title ~~Enable xarray DataArray mtype as sctpye series.~~ [ENH] Enable xarray DataArray mtype as sctpye series. Aug 15, 2022

benHeid force-pushed the feature/2655_enable_xarray branch 2 times, most recently from 7eb4ad4 to 81c4aec Compare August 15, 2022 10:55

Add linting proposals

4654248

benHeid force-pushed the feature/2655_enable_xarray branch from 81c4aec to 4654248 Compare August 15, 2022 10:59

benHeid added 2 commits August 15, 2022 14:28

Fix error with store in _convert.py

bd2ea13

Apply black

ea4ba50

* Build xr.Dataarray manually instead of using the convenience functi…

8ebe25b

…ons which swap the dimensions.. * Append deep_equals for xarray testing.

benHeid commented Aug 16, 2022

View reviewed changes

fkiraly requested changes Aug 16, 2022

View reviewed changes

benHeid added 3 commits August 19, 2022 10:02

* Fix univariate check

bc60a9b

* Enable xarray with only one dimension (Explicit transformation to pandas DataFrame instead of using to_pandas) * Add identity transformation from xr.DataArray to xr.DataArray

* Change checking of xr.DataArrays in deep_equals.py

63df205

* Fix linting errors

26b5227

Simplify xr.DataArray examples (Use default names for dims)

de2ba66

fkiraly changed the title ~~[ENH] Enable xarray DataArray mtype as sctpye series.~~ [ENH] support for xarray DataArray & mtypes Aug 19, 2022

fkiraly reviewed Aug 20, 2022

View reviewed changes

sktime/utils/_testing/deep_equals.py Show resolved Hide resolved

fkiraly mentioned this pull request Aug 20, 2022

[ENH] Unit test for back converter from Gluonts to Multindex DF #3061

Draft

6 tasks

Add a new if case for np.any in deep_equals.py

98f4255

benHeid marked this pull request as ready for review August 25, 2022 08:13

benHeid requested a review from aiwalter as a code owner August 25, 2022 08:13

benHeid requested review from fkiraly and removed request for aiwalter August 25, 2022 08:14

fkiraly approved these changes Aug 25, 2022

View reviewed changes

fkiraly mentioned this pull request Aug 26, 2022

[ENH] How to deal with mtypes that have soft dependencies? #2957

Closed

fkiraly merged commit 9ea7f99 into sktime:main Aug 26, 2022

		return concat_fun


		def _extend_conversions(mtype, anchor_mtype, convert_dict):

Uh oh!

Conversation

benHeid commented Aug 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Does your contribution introduce a new dependency? If yes, which one?

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions

For new estimators

Uh oh!

benHeid commented Aug 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODOs

Uh oh!

fkiraly commented Aug 13, 2022

Uh oh!

fkiraly commented Aug 14, 2022

Uh oh!

benHeid commented Aug 15, 2022

Uh oh!

benHeid commented Aug 15, 2022

Uh oh!

fkiraly commented Aug 15, 2022

Uh oh!

benHeid commented Aug 15, 2022

Uh oh!

benHeid commented Aug 15, 2022

Update:

Uh oh!

fkiraly commented Aug 15, 2022

Uh oh!

fkiraly commented Aug 16, 2022

Uh oh!

Uh oh!

Uh oh!

benHeid Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

fkiraly Aug 16, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

benHeid commented Aug 16, 2022

Uh oh!

fkiraly commented Aug 16, 2022

Uh oh!

fkiraly left a comment

Choose a reason for hiding this comment

Uh oh!

benHeid commented Aug 19, 2022

Update

Uh oh!

Uh oh!

fkiraly left a comment

Choose a reason for hiding this comment

Uh oh!

benHeid commented Aug 26, 2022

Uh oh!

benHeid commented Aug 26, 2022

Uh oh!

fkiraly commented Aug 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

benHeid commented Aug 13, 2022 •

edited

Loading

benHeid commented Aug 13, 2022 •

edited

Loading