improved defaults and improved flow demonstration #1242
Conversation
| # TODO: Check if there is a formal test for type of seasonality | ||
| args = {"sp": sp, "seasonal": "mul"} if seasonality_present else {} | ||
| # Add irrespective of whether seasonality is present or not | ||
| args.update({"trend": "add"}) |
There was a problem hiding this comment.
We can just do args["trend"] = "add"
There was a problem hiding this comment.
Is this faster than update?
There was a problem hiding this comment.
Yes - though of course here it would not make any difference. Update has you create a new dictionary, which is extra overhead
There was a problem hiding this comment.
Ok, I have updated it now.
| tune_distributions = {} | ||
| if seasonality_present: | ||
| tune_grid = { | ||
| "error": ["add", "mul"], |
There was a problem hiding this comment.
can we have the common part outside? btw. you can merge two dicts with dict_merge = {**dict_a, **dict_b} - the second dict will overwrite the first one in case of same keys
There was a problem hiding this comment.
@Yard1, @TremaMiguel also recommended to have class methods to get the tune_grid and tune_distributions. I think we can combine it with that improvement.
There was a problem hiding this comment.
Plus I prefer it to be explicit so it is easier to read :). Let's discuss in the next meeting.
There was a problem hiding this comment.
Sure thing, breaking it up is a good idea. We can leave it like that for now then
|
Just one thing, next meeting I will like to discuss if there's a better way to test the tuning methods, currently the CI takes more than an hour and most of the time testing the time_series module. Here is an image of |
Related Issuse or bug
Fixes:
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Unit test pass
Checklist: