[BUG] fix behaviour of FourierFeatures with pd.DatetimeIndex

[eenticott-shell]

Reference Issues/PRs

Fixes #3605

What does this implement/fix? Explain your changes.

Adds an extra case into FourierFeatures for dealing with a pd.DatetimeIndex. These are stored in nanoseconds and so the seasonalities have to also be specified in nanoseconds. Wanted to avoid this and work from frequency instead for the DatetimeIndex case.

What should a reviewer concentrate their feedback on?

Any other comments?

[ltsaprounis]

Thanks for the bugfix! left a few comments, but nothing major.
I'm keen to see @fkiraly's thoughts on my comment here, before we merge. I either misinterpreted the functionality of the tag "enforce_index_type", or there's another bug in the tag behaviour.

[ltsaprounis]

Based on this fix and on the discussion on #3605, I suggest we add pd.DateTimeIndex in the enforce_index_type list

[eenticott-shell]

So the real bug was in the enforce index! I've added pd.DatetimeIndex as an allowed type, and addressed the other changes you suggested. Also changed the logic when choosing the frequency to avoid so many nested if statements.

[ltsaprounis]

Looking good!
I left a few suggestions in the comments.

[ltsaprounis]

Tests pass now!
One last thing, can you just add an extra test in the test_fourier.py with something similar to your example in the issue?

Just parametrise the test and add a Y_datetime.
If you specify a monthly freq, the outputs etc should be the same as in the existing test.

[ltsaprounis]

@eenticott-shell - In the case where the index is datetime, will the transformed series index maintain the datetime index, or will it change to period? Cause I think the transform method might be changing the index permanently here. Can you quickly check and maybe add it in the test that the index is not changed. Something like:

from pandas.testing import assert_index_equal
assert_index_equal(left, right, exact=True)

[ltsaprounis]

Perfect! that should be good to go now.
Will approve and merge once the CI is done.

[ltsaprounis]

Will approve and merge once the CI is done.

@fkiraly - given it's a bug, can we merge this before the release? your call...

[eenticott-shell]

Thanks for your help! I think one of the tests might fail, I am having trouble running pytest locally but I am currently trying to fix!

[eenticott-shell]

Think that should have fixed it

[ltsaprounis]

the failing test is not related to your transformer:
FAILED sktime/transformations/panel/rocket/tests/test_Rocket.py::test_rocket_on_gunpoint - assert 0.9933333333333333 == 1.0

Left some comments for avoiding a potential side effect in the transform index and removing the redundant index copy in fit.

[fkiraly]

@fkiraly - given it's a bug, can we merge this before the release? your call...

You need to approve and resolve conversations, @ltsaprounis, if that's what you advocate.

[GuzalBulatova]

Looks good overall, thank you @eenticott-shell ! Have only one question

[GuzalBulatova]

@ltsaprounis could you please resolve the pending changes if you're satisfied with them?

[ltsaprounis]

Looking good! thanks for the fix and your patience with the change requests @eenticott-shell! Will merge once the tests are done.