Skip to content
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

Bug Report: Error encountered when the covariates' size is different in N-Hits #1071

Closed
zhangguangxun opened this issue Jul 25, 2022 · 5 comments

Comments

@zhangguangxun
Copy link

  • PyTorch-Forecasting version:0.10.1
  • PyTorch version:1.12.0
  • Python version:3.8
  • Operating System:Linux version 4.19.91-007.ali4000.alios7.x86_64

Abstract

As #1065 issus metioned, N-HiTS encounters a RuntimeError when using 'time_varying_unknown_reals' and 'time_varying_known_reals' covariates simultaneously. The reason of that error is becasue the dimension of the full-connected network is mismatched during the network define process and tensor forward process.

Error Location In Source Code

In pytorch_forecasting.models.nhits.sub_modules.py From line 124 to line 128, The Linear layers' dimension is defined by

self.hidden_size = [
            self.context_length_pooled * self.output_size
            + (self.context_length + self.prediction_length) * self.covariate_size
            + self.static_hidden_size
        ] + hidden_size

The covariate_size is input and is defined in pytorch_forecasting.models.nhits.__init__.py From line 200 to line 208

def covariate_size(self) -> int:
        """Covariate size.

        Returns:
            int: size of time-dependent covariates
        """
        return len(set(self.hparams.time_varying_reals_decoder) - set(self.target_names)) + sum(
            self.embeddings.output_size[name] for name in self.hparams.time_varying_categoricals_encoder
        )

From the defination of variable self.covariate_size we can figure out the defination is only consider the size of categoricals variables in encoder and the size of reals variables in decoder.

Howerer during the forward process, the dimension of input tensor is defined in pytorch_forecasting.models.nhits.sub_modules.py From line 163 to line 181

        encoder_y = encoder_y.transpose(1, 2)
        # Pooling layer to downsample input
        encoder_y = self.pooling_layer(encoder_y)
        encoder_y = encoder_y.transpose(1, 2).reshape(batch_size, -1)

        if self.covariate_size > 0:
            encoder_y = torch.cat(
                (
                    encoder_y,
                    encoder_x_t.reshape(batch_size, -1),
                    decoder_x_t.reshape(batch_size, -1),
                ),
                1,
            )

        # Static exogenous
        if (self.static_size > 0) and (self.static_hidden_size > 0):
            x_s = self.static_encoder(x_s)
            encoder_y = torch.cat((encoder_y, x_s), 1)

When the dimension of covariates is different between encoder and decoder, Whether categoricals variables or reals variables, it will encounter a dimension mismatched error as #1065 showen.

Fixed Code

The fixed code is in linked https://github.com/zhangguangxun/nhits which defines encoder_covariate_size and decoder_covariate_size in __init__.py and pass them to sub_modules.py to make sure the dimension is matched.

@bendavidsteel
Copy link
Contributor

I also encountered this bug, and made a branch fixing it with the exact same code here before seeing this issue!: https://github.com/bendavidsteel/pytorch-forecasting/tree/weight-size-bug-fix

Hope one of our solutions can get merged!

@manitadayon
Copy link

It seems this problem still persists, can either one of you @zhangguangxun @bendavidsteel opens a pull request or propose your patch as a solution.

@manitadayon
Copy link

@bendavidsteel it seems your fix is not merged yet, do you have any ETA on this? I cannot install the package from your branch (I can do it from master but from your branch, for whatever reason seems to have dependency problem with Pytorch lightening, any suggestion on how to use this while this problem is getting resolved?)

@bendavidsteel
Copy link
Contributor

That's entirely up to @jdb78 at this point! I just merged recent master updates into my branch though, and saw there was an update to requirements.txt, so hopefully the dependency issues will be resolved there?

@bendavidsteel
Copy link
Contributor

The fix just got merged @manitadayon , so this bug can be closed @jdb78

@jdb78 jdb78 closed this as completed Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants