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

Related to issue #112: Exploration failure for weekly data #113

Closed

Conversation

sayanchk
Copy link
Collaborator

@sayanchk sayanchk commented Sep 7, 2022

The current approach for trend turning was enabled only for daily and hourly time series. Added a quick fix to trigger computation of window sizes for other frequency types.

@papaemman
Copy link
Contributor

papaemman commented Sep 7, 2022

Hey guys,

I requested this change because I was trying to call that function, and it didn't work.

I have a quick question regarding the modification here.

I was expecting something like this to change.

{'H':24, 'D':7, 'W':4, 'M':12}

which is the frequency to get to the immediately higher time interval.

Why did you do None for anything higher than 'D'?
What's the meaning of the tc_window_lenght variable?

@sayanchk
Copy link
Collaborator Author

sayanchk commented Sep 7, 2022

@papaemman tc_window_length is a required parameter for the _trend_changes in the DataExploration class. Luminaire also has a backup method _detect_window_size inside the same class that can find out the best window for a given time series based on periodic patterns. The change in the PR is essentially triggering that function by passing None for tc_window_lenght instead of erroring out. Please refer to this section for better understanding.

@papaemman
Copy link
Contributor

@sayanchk I was running some experiments, and the _trend_changes function doesn't return any trends, even in obvious settings.

Only if you give {'H':24, 'D':7, 'W':4}
and self.tc_window_length = tc_window_len_dict.get(freq) if freq in ['H', 'D', 'W'] else None
the trends are meaningful.

Kindly re-check the PR. I think we need to go with the solution I mentioned here, meaning that we need to give a starting point for the tc_window_lenght=4 for the time series with a weekly frequency.

Thank you!

@sayanchk
Copy link
Collaborator Author

sayanchk commented Sep 8, 2022

@papaemman the logic behind tc_window_length is to represent the most obvious periodic pattern in the data. Typically, hourly and daily time series show swings at 24 and 7 unit time intervals respectively (with some exceptions). Although, I am not sure if a near month (i.e. {'w': 4}) seasonality is mostly true for weekly data.

Can you share more about the example that you tried for testing the _trend_changes method?

@papaemman
Copy link
Contributor

Hey @sayanchk,

Sorry for the late response.

I run some experiments setting both W=4 and W=None.

First case: W=4

The code snippet was

tc_window_len_dict = {
    'H': 24,
    'D': 7,
    'W':4
}
self.tc_window_length = tc_window_len_dict.get(freq) if freq in ['H', 'D', 'W'] else None

This gave me reasonable trends and changepoints for multiple time series. For example,

Trends_and_changepoints_with_W=4

Second case: W=None

The code snippet was

tc_window_len_dict = {
    'H': 24,
    'D': 7
}
self.tc_window_length = tc_window_len_dict.get(freq) if freq in ['H', 'D'] else None

This didn't return any trends and changepoints at all for multiple time series (even for obvious settings). For example,
Trends_and_changepoints_with_W=None

Conclusion

In conclusion, I believe that this PR isn't correct, and we need to go with the W=4 solution. I create a new PR for this.


You can check more in my repository here: https://github.com/papaemman/luminaire_demo

Thanks,
Panos

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.

None yet

2 participants