Feat/MIDAS transformer #1820
Merged
Feat/MIDAS transformer #1820
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…series through 'ts_transform()'
…series through 'ts_transform()'
…like they should in case of a MIDAS transformation.
…for every quarter instead of there being some missing months, also changed the docstring a bit such that it works with the debugger
|
@madtoinou I just included the strip argument for convenience, however it indeed can lead to unexpected behaviour. More fool-proof to remove that argument, agreed! Thank you very mich for finishing this job and doing at such an impressive pace! |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #1820 +/- ##
==========================================
+ Coverage 93.85% 93.90% +0.05%
==========================================
Files 134 135 +1
Lines 13123 13283 +160
==========================================
+ Hits 12317 12474 +157
- Misses 806 809 +3 ☔ View full report in Codecov by Sentry. |
…the anchor value.
dennisbader
force-pushed
the
feat/add_midas_transformer
branch
from
6ea8ba4
to
68cfa01
Compare
dennisbader
approved these changes
Nov 18, 2023
There was a problem hiding this comment.
I made some updates:
- probabilistic supports
- leverage sliding windows for MIDAS.transform() instead of iterating through the dataframe
- reduce upsampling to a minumum
Thanks again and congrats for this @Beerstabr and @madtoinou!
It was a long way, but now it's ready to be merged :) 💯 🚀
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1570.
Summary
Based on @Beerstabr PR #1668, simplified some part of the code and made the transformer invertible.
The original high frequency is imputed based on the number of component introduced by the
transform()method and the low frequency name (which is used to generate apd.Timedelta). We could also allow the user to hint the target frequency by adding an argument to the function.When the ratio between the low and the high frequency is not a fixed number (number of days in a quarter for example, after doing from months to quarter), the code relies on a rule-based logic. For anchored offset (
W-SUNfor example), the start time of the returnedTimeSeriesis not always identical to the originalTimeSeriesso I added anoffsetargument to allow the user to adjust for itOther Information
Not sure if the
stripparameter should be set toTrueby default or even part of the transform as it can induce counter-intuitive results. @Beerstabr, did you include it for any specific reason?