Skip to content

Data types module, collating conversions, mtype inference, checks, register#1201

Merged
fkiraly merged 14 commits intomainfrom
data-types
Jul 28, 2021
Merged

Data types module, collating conversions, mtype inference, checks, register#1201
fkiraly merged 14 commits intomainfrom
data-types

Conversation

@fkiraly
Copy link
Copy Markdown
Collaborator

@fkiraly fkiraly commented Jul 24, 2021

This PR collates data type and conversion related concerns in its own module, datatypes.

The functionality moved to datatypes:

  • series conversion functionality formerly in forecasting.base.convertIO
  • panel conversion functionality formerly in utils.data_processing._panel

All references to the old locations have been replaced by references to the new locations.

New features enabled by this move:

  • there is just a single convert function, for both Series and Panel scitype
  • nested, extensible module structure which is by scitype, e.g., panel and series

New functionality, partially implemented for extension:

  • register of mtypes with explanation, to be complemented by docs
  • central fixture generation function get_example which produces fixtures of the same "scientific content" but a specific mtype, for external tests, and bulk testing of conversion or mtype inference functionality

@review-notebook-app
Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@fkiraly fkiraly requested review from RNKuhns and chrisholder July 24, 2021 17:44
@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 24, 2021

I guess I win the "maximize number of codeowners who have to review" game with this one...

Copy link
Copy Markdown
Contributor

@TonyBagnall TonyBagnall left a comment

Choose a reason for hiding this comment

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

I've discussed this with my group. This is definitely something that needs doing, and whilst the style is not mine, it fine.

  1. I would ask you have some explanation of machine types, scitypes, even if just linked from the docstrings.

2)I'm also not 100% sure it needs a full root level module, rather than being in data (scikit style) or utilities (my preference). I also am trying to think of another name, I dont like datatypes.

  1. I dont like the mix of underscore and camel case naming conventions,

That all said, in order to not hold up transformers, I'm fine with it going in

@fkiraly
Copy link
Copy Markdown
Collaborator Author

fkiraly commented Jul 26, 2021

I would ask you have some explanation of machine types, scitypes, even if just linked from the docstrings.

Yes, I wanted to write a doc page.

I'm also not 100% sure it needs a full root level module, rather than being in data (scikit style) or utilities (my preference). I also am trying to think of another name, I dont like datatypes.

Neither am I, I just moved different things from different internal places in one place. Once we come up witih something better, we can deprecate this. Also, "mtypes" or "sciptypes" would be too cryptic as a name...

I dont like the mix of underscore and camel case naming conventions,

My linter didn't complain...

@fkiraly fkiraly merged commit 2f111fa into main Jul 28, 2021
@fkiraly fkiraly deleted the data-types branch July 28, 2021 08:27
fkiraly added a commit that referenced this pull request Aug 26, 2021
…rs (#1225)

This PR further consolidates the datatypes module introduced in #1061 and #1201:

* adding example fixtures for most important panel data containers
* consolidated checks for "is (some type)" in module, added some missing ones
* adding registry constants for panel data containers
* renaming leftover "what" variables to "obj"
* bugfix in converter from nested to multi-index
* adding converters for list-of-data-frames panel type used in the distance module
* added tests for checking functionality
* adding tests for conversion functionality, testing converters against fixtures
* adding docstrings where they were missing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants