[ENH] Create a unified Test framework and a Base class for Data Modules in v2

[Faakhir30]

Reference Issues/PRs

Fixes #2298

What does this implement/fix? Explain your changes.

• Create BaseTimeSeriesDataModule in _base_data_module.py with shared init, split, setup template, metadata cache, dataloader factories (implement private abstract methods in child classes for DM specific logic)
• Refactor EncoderDecoderTimeSeriesDataModule and TslibDataModule to inherit base
• Add _BasePtDataModule, EncoderDecoderDataModule_pkg, TslibDataModule_pkg (tags and test param factories)
• Create test_all_data_modules.py + _datamodule_config.py using BaseFixtureGenerator
• Register TestAllDataModules in test_class_register.py
• Migrate duplicate tests and remove/slim test_data_module.py and test_tslib_data_module.py
• Update/Add extension_templates/v2/datamodule/ stub
• Remove spliting of indices logic
• restructure files in data_modueles

Did you add any tests for the change?

Yes, test_all_data_modules.py

PR checklist

• The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
• Added/modified tests
• Used pre-commit hooks when committing to ensure that code is compliant with hooks. Install hooks with pre-commit install.
  To run hooks independent of commit, execute pre-commit run --all-files

[codecov]

Codecov Report

❌ Patch coverage is 96.40180% with 24 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@9ad9989). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
pytorch_forecasting/tests/test_all_data_modules.py	96.07%	13 Missing ⚠️
...ting/data/data_module/base/_base_datamodule_pkg.py	79.16%	5 Missing ⚠️
...casting/data/data_module/base/_base_data_module.py	96.96%	3 Missing ⚠️
...module/encoder_decoder/_encoder_decoder_dataset.py	98.21%	1 Missing ⚠️
...recasting/data/data_module/tslib/_tslib_dataset.py	98.03%	1 Missing ⚠️
pytorch_forecasting/tests/_data_scenarios.py	94.73%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2313   +/-   ##
=======================================
  Coverage        ?   87.57%           
=======================================
  Files           ?      178           
  Lines           ?    10197           
  Branches        ?        0           
=======================================
  Hits            ?     8930           
  Misses          ?     1267           
  Partials        ?        0           

Flag	Coverage Δ
cpu	87.57% <96.40%> (?)
pytest	87.57% <96.40%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[phoeenniixx]

Thanks! and welcome to pytorch-forecasting.
This looks nice!

• I think we need a dataset class in BaseDataModule? Or not?
• The setup etc methods can be data module specific, the base class is not able to handle those, which is expected. So, I think we should not remove them, or is there a way to generalise them?
• I think the data set of EncoderDecoderTimeSeriesDataModule could also go out of the class (like TsLibDataModule)?
• Please create separate folders for base, encoder-decoder and tslib and then add those data modules and pkg class to that.
• Also, for time being, we should keep the test files for both data modules, we need to make sure we dont miss any test coverage!

[phoeenniixx]

Why are we importing the data scenarios here? I think that should happen inside the test framework and not at the pkg level?

[Faakhir30]

sure, we can uncouple this thing, but then we will have to maintain a map of datamodule batch_format tag to a D1 TimeSeries returning function used by tests.

currently done at _datamodules_config.py

Edit: should'nt pkg layer be responsible for describing "how to test" the class it encapsulates? was above comment made because of coupling test framework code _data_senarios to pkg layer?

[phoeenniixx]

If we have this method, why do we have self.train_windows etc in __init__, I think this method would return the windows no? We dont need to create them at the initialization, but only when we actually need it - during data loading etc.

[Faakhir30]

why do we have self.train_windows etc in init

At init attributes are just being assigned as None to avoid hasattr checks that data modules originilay had. e.g., if not hasattr(self, "test_dataset"):

but only when we actually need it - during data loading etc.

If this comment is suggesting to avoid saving datasets and windows as attributes at all, then reason behind saving them is caching behavior of windows/datasets in setup call, we wont be able to cache creation of dataset object without saving them.

[phoeenniixx]

Why are we removing this setup? The setup of BaseDataModule is not enough and is very generic - which is a good thing. But the children could (and should) have their own specific setup when needed..

[Faakhir30]

The setup of BaseDataModule is not enough

For tslib and enc-dec data module, I think it is sufficient apart from spliting indices logic that we have removed for now.
My knowledge is limited to these two data modules as I have only seen these two, only thing different in setup of these two is dataset creation, and I do have that smaller abstract method _build_dataset overriden in both subclases.
So, atleast in these two cases, idts subclasses needs thier own setup.

[phoeenniixx]

Hmm, I see, for these classes setup generalisation worked, but that would not be the case when we add FMs to this package - FMs tend to handle data in a bit different way. But that is not an issue, we would just override this default behaviour. For now, if we are able to generalise, then it is a good thing! Nice!

[phoeenniixx]

I think it is sufficient apart from spliting indices logic that we have removed for now.

I think we could create some abstract methods for splitting and windowing and then the child classes create those? Would that make sense?
I think windowing can how they are handled can be different for different types of model families (or datamodules). What do you think @agobbifbk @Faakhir30 @fkiraly

[Faakhir30]

windowing already has abstractmethod _create_windows that is implemented by subclasses.
will do same for splitting in next commit, thanks

[Faakhir30]

I think we need a dataset class in BaseDataModule? Or not?

Not required in current approach, i'm leaving _build_dataset as abstract method to offload dataset creation to subclasses, idts its possible to generalize __getitem__ of dataset classes and have a base class attached as default dataset class for BaseDataModule.
A previous comment regarding refactoring of datasets #2298 (comment)

The setup etc methods can be data module specific, the base class is not able to handle those, which is expected. So, I think we should not remove them, or is there a way to generalise them?

Addressed at #2313 (comment)

I think the data set of EncoderDecoderTimeSeriesDataModule could also go out of the class (like TsLibDataModule)?

Please create separate folders for base, encoder-decoder and tslib and then add those data modules and pkg class to that.

Also, for time being, we should keep the test files for both data modules, we need to make sure we dont miss any test coverage!

Done, tests will be failing for now as old files are having indices/windowing logic etc that we removed in newer design, thanks!

[phoeenniixx]

Done, tests will be failing for now as old files are having indices/windowing logic etc that we removed in newer design

If we are removing it from the base class, why not add them back to the child classes?
When I meant we plan to "remove" those, I meant in the future, not in this PR. We need to replace that part of the code with some cleaner and more advanced version, but that is still a WiP. So, unless we have a clear picture of what to do, I suggest, leaving those parts as it is as they are on main.

[phoeenniixx]

Thanks!
It is moving in a good direction!
I think we can reduce the redundancy here though. I think there are a lot of things that both these datamodules do in a very similar way. So, in the tests, they should be tested in a simiar way and under same tasks as well. In future, if some datamodule deviates from this pattern, we could add the new tests then

[phoeenniixx]

why are we making two different scenarios for the data modules? I think they both should be able to handle the similar type of TimeSeries class?
Also, Is the already avaiable method to create TimeSeries not enough? Do we really need to add new methods for this?

[Faakhir30]

Also, Is the already avaiable method to create TimeSeries not enough? Do we really need to add new methods for this?

in data senarios, i dont see any function returning a single generic TimeSeries, there is one to return three timeseries (train, test, val)

I think they both should be able to handle the similar type of TimeSeries class?

originaly had two functions as test_tslib_data_module and test_data_module.py had seperate timeseries for testing, but yes, we can unify this to single timeseries, done

[phoeenniixx]

would it be better to have tests:skip_by_name tag here? that is what we use for the test framework for the models

[Faakhir30]

I do like tag approach, it provides better dev experience, also original reason why we went in that direction in sktime repo. sktime/sktime#8496

• As I read through codebase, I am seeing we are not documenting/maintaing tags in registry/_tags unlike sktime /skpro, is this intentional? or a future item? (i'd like to work on it if possible)

• I also see that ptf project has almost created convention of EXCLUDED_TESTS in other pkgs testing framework too, we should add tests:skip_by_name support to all of them? (will open seperate PR for this, if maintainers approves, otherwise will fix for only data modules tests here)