[BUG] fix HolidayFeatures crashes if dataframe doesn't contain specified date

[fnhirwa]

fixes #6399

@fkiraly this is my initial PR on the Holidays bug I wanted to clearly understand if using warning to give the user warnings instead of breaking the forecaster is an optimal solution here.

[fkiraly]

This looks like the right approach to me,
(generally, it's useful if you describe explicitly what it is in the first post in the PR, but I think I understand it from the code)

If I understand correctly, please confirm, you are:

• changing exception upon holiday not in the window to a warning
• correcting the condition that leads to the entire period pre/post holiday not intersecting th observed date period

I have some comments about the design:

• I would make it an option whether a warning is raised, or simply nothing happens, and I would make "nothing happens" a default
• warning and error messages should always contain the source, in this case the HolidayFeatures class
• not sure if it easy, I was wondering given the user example, whether there could be a simpler way to specify periodic dates, e.g., Dec 25 every year (without having to copy multiple instances here). Might be the box of pandora though, as you have to deal with leap years etc.

Finally, for all changes, the docstring should be precise about the usage, and a test should be included to ensure the bug case is now working as expected.

[fnhirwa]

This commit moves all private methods accessing class instances inside the class. Made the warnings optional and indicated that in the docstrings, also added a unit test for the bug that was reported. Regarding the

• not sure if it easy, I was wondering given the user example, whether there could be a simpler way to specify periodic dates, e.g., Dec 25 every year (without having to copy multiple instances here). Might be the box of pandora though, as you have to deal with leap years etc.

Regarding this I may have an idea could we introduce a new private instance attribute named _recurring_holidays in a list and have a method that adds these holidays to the instance something like add_reccuring_holidays? With this when generating holidays we can simply check if the elements of self._recurring_holidays are in the window. Not quite sure if this could be the right approach.

[fkiraly]

This commit moves all private methods accessing class instances inside the class.

Why are we doing this? Seems like a major piece of surgery.

I think it is better style if the functions remain pure and outside the class, they are hard to read already:
https://en.wikipedia.org/wiki/Pure_function
In particular, as the change would shorten the docstrings and remove crucial information, I do not think it is an improvement.

I would suggest to revert the changes in structure and focus on changes in logic instead.

Please let me know if you think I am missing something here, or if you disagree.

[fkiraly]

(sorry, accidentally clicked the "close" button, this was unintended, wanted "comment")

[fkiraly]

Regarding this I may have an idea could we introduce

I would suggest focusing on the bugfix for now, as opposed to adding new features (e.g., support for recurring dates or periodic events). The bug occurs for fixed holidays already, I would suggest we try to fix that issue first?

[fkiraly]

Great, looks like the main issue is indeed resolved!

• could you use sktime warn, so you do not need to pass the instance and the warning flag around?
• could you also explain the block I marked, I do not get the condition here. This can be simply me not understanding it, so an explanation is appreciated.

[fkiraly]

@fnhirwa, could you kindly resolve the code formatting failures? In general, please do so before marking a PR ready for review in the standup.

[fnhirwa]

@fnhirwa, could you kindly resolve the code formatting failures? In general, please do so before marking a PR ready for review in the standup.

This failure is not due to my changes. Related to the runner environment🙄

[fkiraly]

oh, did we update precommit since then?

[fkiraly]

Thanks for the explanation. Looks good to merge from my side.

[fnhirwa]

oh, did we update precommit since then?

no we didn't sometimes the runners fail with no reason