greykite icon indicating copy to clipboard operation
greykite copied to clipboard

In get_auto_silverkite_model_template, `min_gap_in_seconds` fails if input is not sorted

Open rodonn opened this issue 1 year ago • 1 comments

The function min_gap_in_seconds has a comment that it expects the input to be sorted, however this is not clearly communicated in the calling functions. Failing to sort the input table, causes the period value to be incorrectly calculated.

I can think of a few options for how to resolve:

  1. Add a test in min_gap_in_seconds that gives an informative error if the input is not sorted
  2. Automatically sort the table
  3. Add a comment in run_forecast_config telling users the table needs to be sorted.

I'm happy to make a PR that implements any of these options, but figured it would be better to ask which option you would prefer before making the changes.

I'd recommend option 1, since I think an unsorted input table might reflect an error on the user's side, for example it might be a table that includes multiple time series stacked together into one table.

rodonn avatar Mar 27 '23 16:03 rodonn

Hi! Thanks so much for your question and this is a very good catch! We took a look following your question and see that when unsorted time series is passed in, the library does not do a proper inference on the data frequency at some part. We are planning to fix it in apply_forecast_config and will add a test to the function for this purpose too. It’s put in our backlog now.

Thanks again for your help and support!

amyfei2015 avatar May 18 '23 21:05 amyfei2015