Skip to content

Conversation

phoeenniixx
Copy link
Member

Adds Tide model from dsipts to ptf-v2

Created a new folder tide_dsipts in tide that contains all the necessary parts for the tide

@fkiraly
Copy link
Collaborator

fkiraly commented Aug 10, 2025

I think this PR is ready for review

then please click the button "ready for review" (below), I almost did not see this

@phoeenniixx phoeenniixx marked this pull request as ready for review August 10, 2025 17:53
@agobbifbk
Copy link

Here I have some doubts: are you sure all the keys are in the batch dict: encoder_cont,decoder_cont,encoder_cat,decoder_cat? I see in the D2 that you put some defaults if this information is missing (see

)

This is has not been tested, for example here (

cat_n_embd = self.get_cat_n_embd(cat_vars)
) I don't know what could be the behavior of this operation in case of a 0-shaped tensor. Are your tests finishing without any error?

@phoeenniixx
Copy link
Member Author

phoeenniixx commented Aug 11, 2025

Are your tests finishing without any error?

Yes,
From what I can understand, embedding_cat_variables is to get embeddings for categorical variables, but as we discussed, we are for now, not passing any categoricals, so I think this layer is not used, that's why there is no issue?

The data i am using, doesnot pass any categoricals:

@agobbifbk
Copy link

Ok, I see. Even though there is the key in the dict there is no category to loop on. It should work :-)

@phoeenniixx
Copy link
Member Author

exactly :)

@jgyasu jgyasu moved this from PR in progress to PR under review in May - Sep 2025 mentee projects Aug 11, 2025
@@ -430,8 +430,8 @@ def __getitem__(self, idx):
encoder_indices = slice(start_idx, start_idx + enc_length)
decoder_indices = slice(start_idx + enc_length, end_idx)

target_scale = data["target"][encoder_indices]
target_scale = target_scale[~torch.isnan(target_scale)].abs().mean()
target_past = data["target"][encoder_indices]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we doing this? This seems like a significant change to the internal contract of TimeSeriesDataSet

Copy link
Member Author

@phoeenniixx phoeenniixx Aug 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed it some months back, that there was no target_past in EncoderDecoderDataModule and some models do require it. TSLibDataModule already implements it as history_target (see here). Tide also needs it, and we were already calculating target_past for the target_scale, so I just renamed the variable and added it to the return

(if you see line 509, we are still returning target_scale as well)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I suppose this happens only in D2, so that does not compromise the original design, since D2 could return anything

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exactly :)

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • see above, it seems like there is a significant change to EncoderDecoderTimeSeriesDataModule in terms of the dict return. The target_scale variable might not be assigned, and its source is being changed. Can you please explain?
  • Can you add the new layers in folders sorted by type of layer rather than by name of dependency (dsipts)?

@phoeenniixx phoeenniixx moved this from PR under review to In Progress in May - Sep 2025 mentee projects Aug 18, 2025
@phoeenniixx phoeenniixx moved this from In Progress to PR in progress in May - Sep 2025 mentee projects Aug 18, 2025
@jgyasu jgyasu moved this from PR in progress to PR under review in May - Sep 2025 mentee projects Aug 25, 2025
@phoeenniixx phoeenniixx requested a review from fkiraly August 25, 2025 12:51
Copy link
Collaborator

@fkiraly fkiraly 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 explanation.

@fkiraly fkiraly changed the title [ENH] Add Tide to test framework of ptf-v2 [ENH] Tide model in v2 interface Aug 25, 2025
@fkiraly fkiraly merged commit 6074011 into sktime:main Aug 25, 2025
35 checks passed
@github-project-automation github-project-automation bot moved this from PR under review to Done in May - Sep 2025 mentee projects Aug 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants