Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Support for panel-to-series transformers, merger transformation #5351

Merged
merged 14 commits into from Dec 25, 2023

Conversation

benHeid
Copy link
Contributor

@benHeid benHeid commented Oct 3, 2023

Reference Issues/PRs

fixes #5350

What does this implement/fix? Explain your changes.

It implements a merger to create a single time series out of a panel. To enable it , it introduces Transformers that require Panels as input and transform them to Series. Thus the following adaption to base classes are needed:

  • Determine the output scitype. Thus, in base.py convert_output an elif/else are added.
  • In _convert_output of base.py, a check is added if the input conversion was executed based on incompatibilities to the scitype required by the algorithm or not. If due to incompatibilities, we assume that the output type of the algorithm should not be changed! Otherwise, we assume that this conversion is done because of the input data provided!

Moreover, the expected scitype for the tests is adapted to reflect the new kind of transformer!

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

No

What should a reviewer concentrate their feedback on?

  • Is there a better name for this transfomer?
    • I am unsure if the name is really telling what the transformer does.

Did you add any tests for the change?

I added additional test params

For all contributions
  • I've added myself to the list of contributors with any new badges I've earned :-)
    How to: add yourself to the all-contributors file in the sktime root directory (not the CONTRIBUTORS.md). Common badges: code - fixing a bug, or adding code logic. doc - writing or improving documentation or docstrings. bug - reporting or diagnosing a bug (get this plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
    See here for full badge reference
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
For new estimators
  • I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
  • I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.

@benHeid
Copy link
Contributor Author

benHeid commented Oct 9, 2023

Open Questions:

  • Currently, the merger assumes that the instances are forecasts and thus, it applies a temporal alignment.
    • Shoud this be more general? Enabling no temporal alignment for example?
    • A Panel with shape (10, 3, 5) would those result in a series with (14, 3). Is this intuitive, should there be an option that the nans are handled differently? Currently it is the nanmedian/nanmean behaviour of numpy.

@fkiraly
Copy link
Collaborator

fkiraly commented Oct 9, 2023

Currently, the merger assumes that the instances are forecasts and thus, it applies a temporal alignment.

Could you kindly explain that with a bit more formalism? What is special about the assumption that instances are forecasts?

If I look at the docstring, I do not see where this assumption would come in. I also do not get what you mean by alignment, there is no alignment in the sense of the alignment module that I can spot?

A Panel with shape (10, 3, 5) would those result in a series with (14, 3)

Can you explain why?

is there maybe a docstring that would help?

@benHeid
Copy link
Contributor Author

benHeid commented Oct 10, 2023

Could you kindly explain that with a bit more formalism? What is special about the assumption that instances are forecasts?

I will change this description. This description is heavily inspired by how the cINN-based forecaster uses the merger and is not generalised. They need not to be forecasts.

If I look at the docstring, I do not see where this assumption would come in. I also do not get what you mean by alignment, there is no alignment in the sense of the alignment module that I can spot?

Mhm, I see alignment is confusing here. I called it alignment also since this description is heavily inspired by how the cINN-based forecaster is using the Merger. It generates for each time step a time series segment of the next n values. These segments are aligned in a way that each column contains values that correspond to exactly one timestamp.
Perhaps with a stride-based explanation as I added in the corresponding issue #5350, this would be less confusing.

A Panel with shape (10, 3, 5) would those result in a series with (14, 3)

Can you explain why?

See the explanation in the corresponding issue I added (#5350).

is there maybe a docstring that would help?

I assume that the examples from #5350 should be added as explanations in the docstring.

…e mtype used by the algorithm natively supports a Panel and Panel since the input is a Panel!
* Add a expected Scitype for Panel as input and Series as Output.
@benHeid benHeid marked this pull request as ready for review November 9, 2023 10:15
@benHeid benHeid mentioned this pull request Nov 17, 2023
6 tasks
@benHeid benHeid added implementing algorithms Implementing algorithms, estimators, objects native to sktime module:transformations transformations module: time series transformation, feature extraction, pre-/post-processing enhancement Adding new functionality labels Nov 17, 2023
Copy link
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.

This looks ok now, but since it is changing the base class, we should:

  • add tests for the base class that cover the new case well - including base use, and using this type of transformer in pipelines
  • have a short walkthrough at one of the next meet-ups, as I do not 100% understand the changes in the base class, and the location is so important that I feel I should

@fkiraly
Copy link
Collaborator

fkiraly commented Nov 26, 2023

(and merge only with minor version change to avoid any accidental breakage with patch version)

@fkiraly fkiraly added the release release related PR label Dec 22, 2023
@fkiraly
Copy link
Collaborator

fkiraly commented Dec 22, 2023

scheduled for 0.25.0

@fkiraly fkiraly merged commit b937b1e into main Dec 25, 2023
39 checks passed
@fkiraly fkiraly deleted the merger_transformation branch December 25, 2023 22:30
@fkiraly fkiraly changed the title [ENH] Add Merger Transformation [ENH] Support for panel-to-series transformers, merger transformation Dec 25, 2023
fkiraly pushed a commit that referenced this pull request Feb 22, 2024
## Note relies on 
* DeepLearning base design is merged 
* CurveFitForecaster is merged
* Merger #5351 

#### Reference Issues/PRs
Fixes #4924

#### What does this implement/fix? Explain your changes.
It implements the method of the BigDEAL Challenge that won the peak
shape and the peak timing forecasting tasks.

Furthermore, it adapts a bit the base classes for torch forecasters to
make them more flexible.

#### Does your contribution introduce a new dependency? If yes, which
one?
It introduces FrEIA as a dependency. This is a framework for
implementing reversible neural network architectures.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding new functionality implementing algorithms Implementing algorithms, estimators, objects native to sktime module:transformations transformations module: time series transformation, feature extraction, pre-/post-processing release release related PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] Implement a Merger to create a univariate time series out of a Panel
2 participants