Skip to content

[ENH] Test framework for ptf-v2 #1841

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 112 commits into from
Jun 15, 2025
Merged

Conversation

phoeenniixx
Copy link
Contributor

Description

This PR solves #1838
Implements Test framework for v2

@phoeenniixx phoeenniixx requested a review from PranavBhatP June 9, 2025 19:55
@phoeenniixx
Copy link
Contributor Author

Hi @fkiraly, @PranavBhatP, I tried adding _get_test_datamodule_from to the metadata class, please review it and tell me if I am on a right path

@phoeenniixx
Copy link
Contributor Author

  • I added the whole initialisation logic of the datamodule to _get_test_datamodule_from so that any kind of data module can be added to tests without making any changes to _integration
  • changed the make_dataloaders_v2 to make_datasets_v2 as now it is returning TimeSeries dataset in place of whole dataloaders and datamodule - now _get_test_datamodule_from returns the datamodules and dataloaders.
  • now the _integration test only checks if metadata is a dict.

The open issue is: how to do metadata testing for different datamodules?
By metadata testing I mean testing if the metadata is calcualted correctly and passed correctly to dataloaders and model for initialisation.

@fkiraly
Copy link
Collaborator

fkiraly commented Jun 9, 2025

The open issue is: how to do metadata testing for different datamodules?
By metadata testing I mean testing if the metadata is calcualted correctly and passed correctly to dataloaders and model for initialisation.

Hmmm - here is an idea: each model (intermediate base class) could have a _check_metadata that is called from the test.
It passes if the metadata is of expected type, and fails otherwise with a descriptive exception.

For performance reasons, I would not carry out the check in usage, just in the test.

For the metadata generated by D1, this is universal and can simply be tested by D1 tests - all D1 should produce the same metadata.

@phoeenniixx
Copy link
Contributor Author

each model (intermediate base class) could have a _check_metadata that is called from the test.
It passes if the metadata is of expected type, and fails otherwise with a descriptive exception.

Sorry I didn't quite understand what you mean here.

  • If you mean we add _check_metadata to the base class _BasePtForecasterV2 (or its child class) how to add different tests for different metadata - Imagine one model is from EncoderDecoderDataModule and other from TSLibDataModule then, how would _check_metadata would know which metadata to test? We'll need to add the testing logic of both the data modules to the same method, but what if there is one more data module in future? I think that will make the method little bit messy?
class _BasePtForecasterV2(_BasePtForecaster):
    def _check_metadata():
          if EncoderDecoderDataModule:
                    return test_enc_dec_dm()
          elif TSLibDataModule:
                     return test_tslib_dm()
          elif:
               ... # so on?
  • If you mean each MODEL has _check_metadata then there will be duplicate code for all models having same datamodule?

These are my initial thoughts, but I still need your help to understand what exactly is the idea, I'm a bit lost here😅

@fkiraly
Copy link
Collaborator

fkiraly commented Jun 11, 2025

If you mean each MODEL has _check_metadata

That is what I mean

then there will be duplicate code for all models having same datamodule?

... unless you add the check_metadata to a common intermediate base class that all models using that data module inherit from.
(which I did not say but thought would be implied by this design)

@phoeenniixx
Copy link
Contributor Author

Hi @fkiraly, do we need to add something else to this PR? We can add metadata testing in some other PR?

@fkiraly
Copy link
Collaborator

fkiraly commented Jun 13, 2025

I think this is ready - the conflicts with #1886 need to be resolved though.

@phoeenniixx
Copy link
Contributor Author

done

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.

I made some small changes:

  • deduplicate code copy-pasted twice in _conftest - why did you put the same code in two files?
  • revert change to _BasePtForecaster name, the name remains the same until we move to v2
  • changed "metadata" filename to pkg

@fkiraly fkiraly merged commit 300fd90 into sktime:main Jun 15, 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 Jun 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants