[ENH] Restructure codebase: Create separate modules for shared architecture layers.

[PranavBhatP]

Is your feature request related to a problem? Please describe.
Currently, each model in PTF v1 encapsulates it's set of layers under the sub_modules.py file. This is prone to duplication of code and ignores the possibility of shared layers used by various model architectures in the present and for those to be implemented in the future.

Describe the solution you'd like
Creating a separate module for layers and grouping the layers based on functionality is a good step in this direction. Suggestions are welcome. The same logic can be extended to data modules as well, creating a separate folder for implementations of data modules.

Additional context
Some references to existing codebases which group layers in such a manner:
https://github.com/Nixtla/neuralforecast/tree/main/neuralforecast

[PranavBhatP]

FYI @phoeenniixx @fkiraly @agobbifbk

[fkiraly]

Yes, I agree.

My concrete suggestion for layers would be:

• new top-level module layers
• maximally a single layer per file
• optimally, subfolders like attention etc for type of layer
• all files private for now, so we can change this later
• for moved layers from existing modules, if any locations were private, they need to import there as well
  • optionally, we want to go through a deprecatoin cycle and trigger warnings when imports come from the old location https://www.sktime.net/en/stable/developer_guide/deprecation.html
• in the file docstrings, I would also include some information on where it is from

For D2 loader modules, I think a top module might also make sense. However, that feels a bit less clear at the moment, because we only have one example currently. I would suggest to defer this to later.

[phoeenniixx]

I also have a suggestion:
If the tslib implementation and current models in ptf are independent in terms of these "layers" and they donot use each other's implementations, we can create two-halves of the library: One with tslib and one with original ptf models
import statements can be like this:

from pytorch_forecasting.tslib.layers.xyz import abc

and for original models

from pytorch_forecasting.models.xyz import abc

[fkiraly]

@phoeenniixx, sorry to say this, but I strongly disagree with that suggestion. The separation to "tslib" is not architecturally motivated, at least its semantics is not.

Plus, it does not scale well in the extension case. Let's say there will be a third bunch of models that use a common D2 layer. Do we just add more and more top level modules for each?

I think a separation into models, layers, and "D2" (data loaders?) - or something similar - is the more sensible, architecturally implied division.

[phoeenniixx]

I was thinking a structure like sktime.libs like it had a fork of pykalman but yes you are right it will create complications

[fkiraly]

I was thinking a structure like sktime.libs like it had a fork of pykalman but yes you are right it will create complications

That is different, that is a vendor library. It is not architecturally part of sktime, but a separate library.

The pattern should be avoided in coherent libraries - pykalman and the various LLM are vendored because the original libraries were not maintained or were not properly packaged.

[phoeenniixx]

That is different, that is a vendor library. It is not architecturally part of sktime, but a separate library.

The pattern should be avoided in coherent libraries - pykalman and the various LLM are vendored because the original libraries were not maintained or were not properly packaged.

ohh, got it! Thank you for the explanation

[fkiraly]

@phoeenniixx, @PranavBhatP, it might be better to do this earlier than later, given that we will be adding new models.

Does any of you have capacity currently to take this on - e.g., in a stack with a PR that might add new models?

[PranavBhatP]

Does any of you have capacity currently to take this on - e.g., in a stack with a PR that might add new models?

Yes @fkiraly, I can work on this once I complete the D2 pipeline for tslib. I will stack this as a new PR on top of the existing PR implementing the D2.

[PranavBhatP]

@fkiraly, after implementing the layers folder structure, in #1836, I was wondering whether we need to create a "layer" registry to map the layers to the several models they might be used by? It would simplify a layer's visibility to a new user who wants to implement a model and might be searching for a layer similar to his/her requirements.

This change is something that can be deferred to later, once #1836 is merged.

[fkiraly]

I was wondering whether we need to create a "layer" registry to map the layers to the several models they might be used by?

That is a good idea imo, although it needs to be discussed.

Primarily, I was thinking of a layer registry not for the purpose of linking layers to models, but for the purpose of carrying metadata, and for the purpose of testing layers individually.

The linkage can then easily be reconstructed by lookup and "is part of" operations in torch.

I would agree though that we should deal with this after the models.