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

fix wrong time-series alignment and add WA for StatsForcastAutoARIMA #1022

Merged
merged 6 commits into from Oct 20, 2022
Merged

fix wrong time-series alignment and add WA for StatsForcastAutoARIMA #1022

merged 6 commits into from Oct 20, 2022

Conversation

bachng2017
Copy link
Contributor

@bachng2017 bachng2017 commented Oct 16, 2022

a proposed fix for time-series alignment error and issue of StatsForcastAutoARIMA

  1. see [Bug]: inconsistent result for timeseries mindsdb#3234 for detail about the alignment issue
  2. see https://mindsdbcommunity.slack.com/archives/C037482KJ22/p1665911392587569 for more detail about the issue of StatsForcastAutoARIMA

the proposed fix is tested with house-sale tutorial only. It might need more review to consider the impact of the change

Copy link
Member

@paxcema paxcema 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 the PR :)

The workaround is okay. There are two other modifications that I'm not 100% convinced on yet. I will manually test a bit more, but if you have any proof it would be awesome to translate that into a test that captures the failing behavior, so that we can catch this next time that the alignment fails. If you can't, then give me some time and I will 1) merge this and 2) add tests on top.

@paxcema
Copy link
Member

paxcema commented Oct 18, 2022

Update: the test in #1023 leads me to believe the general alignment behavior is correct (StatsForecast bug notwithstanding), and unless we have a test to showcase an edge case that justifies the above changes, I'm inclined to not merge those. I will try to find such a test, but in the meantime @bachng2017, it does seem like the issue is in mindsdb rather than lightwood, so I will check that now.

@bachng2017
Copy link
Contributor Author

thanks @paxcema
did you check the conversation at https://mindsdbcommunity.slack.com/archives/C037482KJ22/p1665911392587569
from that it looks like an issue of StatsForcastAutoARIMA for us

@paxcema
Copy link
Member

paxcema commented Oct 19, 2022

For now, can you please remove the changes in offset indexes?

Just leave the StatsForecast workaround, and I will merge immediately. We can debug the second problem in a separate PR. Thanks!

@bachng2017
Copy link
Contributor Author

yes we've just left the StatsForcastAutoARIMA wa there and remove other changes

@paxcema
Copy link
Member

paxcema commented Oct 19, 2022

Can you try to merge again please? Somehow GitHub thinks the newly added RandomForest mixer will be added again, which should not be the case.

@bachng2017
Copy link
Contributor Author

tried to merge again, not sure is that OK or not

@paxcema
Copy link
Member

paxcema commented Oct 20, 2022

Yes, merge is now fine 👍

There is a failing test, however. I will have to take a closer look on why before merging.

@paxcema
Copy link
Member

paxcema commented Oct 20, 2022

Ah, it fails when min_offset = -np.inf, so this is not enough. I'm looking at alternative solutions.

EDIT: Found a way. I will merge this anyway because it has certainly been very helpful, and I will overwrite on top with another PR. Thanks @bachng2017!

@paxcema paxcema merged commit d36465f into mindsdb:staging Oct 20, 2022
@bachng2017
Copy link
Contributor Author

thanks, the merge also very helpful for us.
We will do our own test (manual) with the latest and try to submit other issue (because we think the index issue which we've removed from this PR still be there)

@paxcema paxcema mentioned this pull request Oct 26, 2022
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

3 participants