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] pytorch-forecasting#1752 Fixing #1786

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

RUPESH-KUMAR01
Copy link

Description

This PR is towards the issue #1752 . concat_sequences concat the timesteps of each batch. But our goal is to not concat time_stamps but to concat the batches.

Checklist

  • Linked issues (if existing)
  • Amended changelog for large changes (and added myself there as contributor)
  • 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

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.

Great - could we add a test to ensure we have fixed the bug?

@fkiraly fkiraly added the bug Something isn't working label Feb 26, 2025
@RUPESH-KUMAR01
Copy link
Author

RUPESH-KUMAR01 commented Feb 27, 2025

Changes Made to include the test for the bug :

  1. Changed fast_dev_run to 2

    • Previously, only one batch was run, which might have hidden errors.
    • Now, two batches will run to ensure better testing.
  2. Added an assertion to check y shape after concatenation

    • Ensures that predictions match target dimensions after processing.

Updated Code Snippet:

predictions = net.predict(
    val_dataloader,
    return_index=True,
    return_x=True,
    return_y=True,
    fast_dev_run=2,  # 🔹 Now runs for two batches
    trainer_kwargs=trainer_kwargs,
)

if isinstance(predictions.output, torch.Tensor):
    assert predictions.output.shape == predictions.y[0].shape, "shape of predictions should match shape of targets"
else:
    for i in range(len(predictions.output)):
        assert predictions.output[i].shape == predictions.y[0][i].shape, "shape of predictions should match shape of targets"

I am not familiar with tests, but while debugging, I found where the tests failed with my previous approach and modified the function with extra constraints.
Location of the file where I modified:
tests\test_models\test_temporal_fusion_transformer.py
function: _integration

If the changes are sufficient I will add the modifications to the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Development

Successfully merging this pull request may close these issues.

2 participants