Skip to content

[BUG] fix incorrect concatenation dimension in concat_sequences#1827

Merged
fkiraly merged 7 commits intosktime:mainfrom
cngmid:issue_1823
May 16, 2025
Merged

[BUG] fix incorrect concatenation dimension in concat_sequences#1827
fkiraly merged 7 commits intosktime:mainfrom
cngmid:issue_1823

Conversation

@cngmid
Copy link
Contributor

@cngmid cngmid commented May 8, 2025

Description

This PR suggest a fix 1823

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

Make sure to have fun coding!

@codecov
Copy link

codecov bot commented May 8, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@5685c59). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1827   +/-   ##
=======================================
  Coverage        ?   86.78%           
=======================================
  Files           ?       51           
  Lines           ?     5668           
  Branches        ?        0           
=======================================
  Hits            ?     4919           
  Misses          ?      749           
  Partials        ?        0           
Flag Coverage Δ
cpu 86.78% <100.00%> (?)
pytest 86.78% <100.00%> (?)

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

☔ View full report in Codecov by Sentry.
📢 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.

@cngmid
Copy link
Contributor Author

cngmid commented May 8, 2025

After my first commit, there was one test failure of the following test with python 3.13
tests/test_models/test_temporal_fusion_transformer.py::test_prediction_with_dataloder_raw

I removed python-version: 3.13 from .github/workflows/test.yml and excluded a numpy warning in pytest.ini. After the new commit, a new failure arise on the Docs build.

Hopefully the reviewers will fix that.

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.

Thanks!

Could you revert your changes to the test.yml? There is only one combination failing, there is no need to remove all of 3.13 tests. I will take care of that separately, just make sure you do not change the file.

Regarding the skip in the pytest.ini, can you explain why we are skipping that?

@cngmid
Copy link
Contributor Author

cngmid commented May 9, 2025

"Regarding the skip in the pytest.ini, can you explain why we are skipping that?"
It was just to reduce the size of the test log. You are right. We cannot ignore it. I put it back.

@fkiraly
Copy link
Collaborator

fkiraly commented May 9, 2025

ok, but what exactly are we skipping? Why did you suggest to do that in the first place?

@cngmid
Copy link
Contributor Author

cngmid commented May 9, 2025

Sorry for any confusion. Let me clarify two points:

  1. Regarding numpy 2.0 warnings: I am no longer skipping these, nor am I suggesting they should be skipped. My previous temporary measure to skip them was purely for convenience.
  2. Regarding current proposals: The only change I am proposing with respect to the main branch is a one-line modification in the file pytorch_forecasting\utils\_utils.py (specifically at line 275).

Hope this clears things up.

@cngmid cngmid requested a review from fkiraly May 13, 2025 07:00
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.

Yes, thanks.

What would be great is if you could add a test as well - if you are not sure how to add tests, simply paste code that fails prior and now passes.

(to add this as a test, add it as a function test_something inside a module starting with "test")

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.

How about the code in #1823?

This is already a great test case:

  • add the original code with an assert for the correct shape
  • add the modification that fails, as well. Simply paste it, and the test passes if the code does not fail.

@fkiraly fkiraly changed the title PR for issue 1823 [BUG] fix incorrect concatenation dimension in concat_sequences May 13, 2025
@cngmid cngmid requested a review from fkiraly May 15, 2025 13:59
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.

Thanks!

@fkiraly fkiraly added the bug Something isn't working label May 16, 2025
@fkiraly fkiraly merged commit fc8599c into sktime:main May 16, 2025
35 checks passed
PranavBhatP pushed a commit to PranavBhatP/pytorch-forecasting that referenced this pull request May 22, 2025
PranavBhatP pushed a commit to PranavBhatP/pytorch-forecasting that referenced this pull request May 22, 2025
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

Status: Fixed/resolved

Development

Successfully merging this pull request may close these issues.

[BUG] utils.concat_sequences concatenation should be on dim=0 ?

2 participants