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

Add optional inverse transform in historical forecast #2267

Conversation

alicjakrzeminska
Copy link
Contributor

@alicjakrzeminska alicjakrzeminska commented Mar 5, 2024

Edit:

Checklist before merging this PR:

  • Mentioned all issues that this PR fixes or addresses.
  • Summarized the updates of this PR under Summary.
  • Added an entry under Unreleased in the Changelog.

Fixes #2260.

Summary

  • InvertibleDataTransformer now supports parallelized inverse transformation for series being a list of lists of TimeSeries (Sequence[Sequence[TimeSeries]]). This series type represents for example the output from historical_forecasts() when using multiple series.


"""

if not forecasts_list_transformed:

Choose a reason for hiding this comment

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

i don't get this point. If forecasts_list_transformed is empty you are doing nothing right?

Can you check that before calling the method?

The method is private, so it should be used within this class.

Make it sense? :-)

return forecasts_list_transformed

if isinstance(forecasts_list_transformed[0], TimeSeries):
forecasts_list_inversely_transformed = scaler.inverse_transform(

Choose a reason for hiding this comment

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

Suggested change
forecasts_list_inversely_transformed = scaler.inverse_transform(
return scaler.inverse_transform(

in this way you don't have to go down in the method to check what is returned in this case :-)

else:
fill_value = TimeSeries.from_values(np.empty(1))

forecasts_list_inversely_transformed = list(

Choose a reason for hiding this comment

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

at this stage here the list is not inversely transformed. Perhaps another name for this variable.

Comment on lines 2123 to 2124
"""Applies inverse transform on the list (of lists) of forecasts.
Uses the scaler trained previously on the same set of series.

Choose a reason for hiding this comment

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

Suggested change
"""Applies inverse transform on the list (of lists) of forecasts.
Uses the scaler trained previously on the same set of series.
"""Applies inverse transform on the list (of lists) of forecasts,
using the scaler trained previously on the same set of series.

Comment on lines 742 to 743
scaler
Optionally, the scaler trained on the same `series`, performs inverse transform on the generated forecasts.

Choose a reason for hiding this comment

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

Suggested change
scaler
Optionally, the scaler trained on the same `series`, performs inverse transform on the generated forecasts.
scaler
Optionally, the scaler trained on the same `series` is used to perform an inverse transform on the generated forecasts.

@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2024

Codecov Report

Attention: Patch coverage is 85.29412% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 93.94%. Comparing base (d764bc4) to head (ff80a7e).

Files Patch % Lines
...taprocessing/transformers/base_data_transformer.py 71.42% 8 Missing ⚠️
...ocessing/transformers/fittable_data_transformer.py 94.11% 1 Missing ⚠️
...essing/transformers/invertible_data_transformer.py 95.65% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2267      +/-   ##
==========================================
- Coverage   94.00%   93.94%   -0.07%     
==========================================
  Files         136      136              
  Lines       13648    13673      +25     
==========================================
+ Hits        12830    12845      +15     
- Misses        818      828      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Thanks for this Draft PR @alicjakrzeminska. I would suggest to move the logic to darts.dataprocessing.transformers.invertible_data_transformer.InvertibleDataTrasnformer. In my opinion this can generally be applied to any InvertibleDataTransfomer. What do you think?

Comment on lines 360 to 361
* [A1, B1, C1] - list of series,
* [[A1, A2, A3], [B1, B2], [C1, C2, C3]] - list of lists of series,

Choose a reason for hiding this comment

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

Suggested change
* [A1, B1, C1] - list of series,
* [[A1, A2, A3], [B1, B2], [C1, C2, C3]] - list of lists of series,
- [A1, B1, C1] - list of series,
- [[A1, A2, A3], [B1, B2], [C1, C2, C3]] - list of lists of series,

Comment on lines 365 to 366
Allows for different lengths of the lists.
Allows for missing series the list.

Choose a reason for hiding this comment

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

Suggested change
Allows for different lengths of the lists.
Allows for missing series the list.
Supports lists of varying lengths and missing elements within the list/s.

@alicjakrzeminska
Copy link
Contributor Author

@dennisbader, I moved the function to InvertibleDataTransfomer . Please, take a look, and if you find it ok, I'll add some tests.
@VascoSch92, thank you for your feedback, I applied your suggestions. :)

@alicjakrzeminska
Copy link
Contributor Author

@dennisbader, I integrated the functionality with inverse_transform method of InvertibleDataTransfomer as you suggested. Please, have a look, and I'll write the docstrings and tests if it's ok for you.

@dennisbader dennisbader marked this pull request as ready for review March 15, 2024 18:06
Comment on lines 332 to 334
for series_ in series_list:
input_series.append(series_)
data.append(series_)

Choose a reason for hiding this comment

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

Suggested change
for series_ in series_list:
input_series.append(series_)
data.append(series_)
input_series.extend(series_list)
data.extend(series_list

Copy link
Collaborator

@dennisbader dennisbader left a comment

Choose a reason for hiding this comment

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

Really nice, thanks a lot for this great PR @alicjakrzeminska 🚀

@dennisbader dennisbader merged commit 91c7087 into unit8co:master Mar 16, 2024
9 checks passed
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.

Inverse scaling for the list of series (different lengths handling)
4 participants