Skip to content

Adapter from multiindex to ListDataset #2976

Merged
fkiraly merged 21 commits intosktime:mainfrom
TNTran92:List_Dataset_3
Jul 18, 2022
Merged

Adapter from multiindex to ListDataset #2976
fkiraly merged 21 commits intosktime:mainfrom
TNTran92:List_Dataset_3

Conversation

@TNTran92
Copy link
Copy Markdown
Contributor

@TNTran92 TNTran92 commented Jul 11, 2022

Reference Issues/PRs

Partial solution to Issue #2860

Continuation from PR# 2893

What does this implement/fix? Explain your changes.

Implement an adapter to convert multiindex format to ListDataset in gluon-ts
The method takes as input a pd-multiindex DataFrame, (optional) a list of categorical feature, (optional) the startdate and (optional) frequency of the dataset.

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

Dependency: gluon-ts

What should a reviewer concentrate their feedback on?

Testing with different dataset in sktime to weed out the bugs. So far, Arrowhead, ItalyPoowerDemand and StandWalkJump have been tested.

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.

@TNTran92 TNTran92 mentioned this pull request Jul 11, 2022
Closed
6 tasks
@TNTran92
Copy link
Copy Markdown
Contributor Author

TNTran92 commented Jul 11, 2022

@fkiraly, I added the unit test to the end of sktime/datatypes/tests/test_panel_converters.py

I'm having some ImportError: cannot import name 'convert_from_multiindex_to_listdataset' from 'sktime.datatypes._adapters' (unknown location) even tho I've already put __init__.py in the _adapter folder. Will try poking around to get this fix.

@ltsaprounis
Copy link
Copy Markdown
Contributor

ltsaprounis commented Jul 12, 2022

Hi Tan!

sktime/datatypes/_adapter/pd_multiindex_to_list_dataset needs to end in .py as well.
If you want to import the convert_from_multiindex_to_listdataset from sktime.datatypes._adapter you need to write the following code in the sktime/datatypes/_adapter/__init__.py file:

__all__ = ["convert_from_multiindex_to_listdataset"]

from sktime.datatypes._adapter.pd_multiindex_to_list_dataset import convert_from_multiindex_to_listdataset

@TNTran92
Copy link
Copy Markdown
Contributor Author

TNTran92 commented Jul 12, 2022

Hi Tan!

sktime/datatypes/_adapter/pd_multiindex_to_list_dataset needs to end in .py as well. If you want to import the convert_from_multiindex_to_listdataset from sktime.datatypes._adapter you need to write the following code in the sktime/datatypes/_adapter/__init__.py file:

__all__ = ["convert_from_multiindex_to_listdataset"]

from sktime.datatypes._adapter.pd_multiindex_to_list_dataset import convert_from_multiindex_to_listdataset

I see. But can you explain what is the reason for having __all__ I've seen it in a number of places in the repo

@TNTran92
Copy link
Copy Markdown
Contributor Author

TNTran92 commented Jul 13, 2022

@fkiraly In my local machine, I have a failure at sktime/classification/interval_based/tests/test_drcif.py::test_contracted_drcif
Which is weird because I don't use this method. Also, it only fails the windows test, can you give me some pointers?

@TNTran92
Copy link
Copy Markdown
Contributor Author

TNTran92 commented Jul 13, 2022

@fkiraly In my local machine, I have a failure at sktime/classification/interval_based/tests/test_drcif.py::test_contracted_drcif Which is weird because I don't use this method. Also, it only fails the windows test, can you give me some pointers?

@fkiraly For some reasons, the tests are now finished. Both the adapter and unit tests are ready.

@TNTran92 TNTran92 marked this pull request as ready for review July 13, 2022 21:26
@fkiraly
Copy link
Copy Markdown
Collaborator

fkiraly commented Jul 13, 2022

excellent! Will look & probably merge after the 0.13.0 release.

fkiraly
fkiraly requested changes Jul 16, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I would merge if not for what looks like an accidental mistake.

I think there is a commit/merge accident, there are two files pd_multiindex_to_listdataset, one without py extension and one with. I suspect you want only the one with.

I am not going to delete it though, since the content seems different - could you kindly have a look?

@TNTran92
Copy link
Copy Markdown
Contributor Author

TNTran92 commented Jul 17, 2022

Looks great! I would merge if not for what looks like an accidental mistake.

I think there is a commit/merge accident, there are two files pd_multiindex_to_listdataset, one without py extension and one with. I suspect you want only the one with.

I am not going to delete it though, since the content seems different - could you kindly have a look?

My bad. The one without .py was supposed to be removed. I have made a new commit.

@TNTran92
Copy link
Copy Markdown
Contributor Author

TNTran92 commented Jul 18, 2022

Added back conversion (unit test in progress)

fkiraly
fkiraly approved these changes Jul 18, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@fkiraly fkiraly left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! wasn't expecting the back-converesion, just wanted to merge.

I suppose the back-conversion is native to nested_univ, so a clean version has that as a return type and does further conversions depending on the desired result type.

Either way, I think this is good to go in in order to unblock @AurumnPegasus, we can refactor and add more unit tests later.

@fkiraly fkiraly merged commit 6bfb298 into sktime:main Jul 18, 2022
@fkiraly
Copy link
Copy Markdown
Collaborator

fkiraly commented Jul 18, 2022

FYI @AurumnPegasus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fkiraly fkiraly fkiraly approved these changes

@aiwalter aiwalter Awaiting requested review from aiwalter

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@TNTran92 @ltsaprounis @fkiraly