Skip to content

Conversation

PranavBhatP
Copy link
Contributor

@PranavBhatP PranavBhatP commented Aug 22, 2025

Reference Issues/PRs

Closes #1940

Copy link

codecov bot commented Aug 22, 2025

Codecov Report

❌ Patch coverage is 84.61538% with 26 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@3821c0b). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...ytorch_forecasting/layers/_normalization/_revin.py 71.73% 13 Missing ⚠️
...orch_forecasting/models/samformer/_samformer_v2.py 83.54% 13 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1952   +/-   ##
=======================================
  Coverage        ?   87.19%           
=======================================
  Files           ?      148           
  Lines           ?     9065           
  Branches        ?        0           
=======================================
  Hits            ?     7904           
  Misses          ?     1161           
  Partials        ?        0           
Flag Coverage Δ
cpu 87.19% <84.61%> (?)
pytest 87.19% <84.61%> (?)

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.

@jgyasu jgyasu moved this to PR in progress in May - Sep 2025 mentee projects Aug 25, 2025
@fkiraly fkiraly added the enhancement New feature or request label Aug 25, 2025
@fkiraly
Copy link
Collaborator

fkiraly commented Aug 27, 2025

FYI @PranavBhatP, working on a check_estimator utility that should help with getting tests to pass: #1954

It seems like this PR is also not up to date with main?

@PranavBhatP
Copy link
Contributor Author

I will be pushing changes in a while.

@PranavBhatP PranavBhatP marked this pull request as ready for review August 27, 2025 12:10
@PranavBhatP
Copy link
Contributor Author

@fkiraly could this be assigned to me? I don't think it will be visible on the kanban board...

@agobbifbk
Copy link

In the original implementation there is this switch:

if hasattr(nn.functional, 'scaled_dot_product_attention'):
            att_score = nn.functional.scaled_dot_product_attention(queries, keys, values) # (n, D, L)
        else:
            att_score = scaled_dot_product_attention(queries, keys, values) # (n, D, L)

in your implementation you are leveraging on nn.functional.scaled_dot_product_attention, can you check if the error persists if you use the scaled_dot_product_attention there was in the original implementation?

If the error persists, we can have a look together!

@PranavBhatP
Copy link
Contributor Author

PranavBhatP commented Aug 27, 2025

works now! i am using the custom implementation.

test for quantile loss case is not included since we do not have a nn metric for quantile predictions
@fkiraly
Copy link
Collaborator

fkiraly commented Aug 28, 2025

In the original implementation there is this switch:

what is this switch for, @agobbifbk? Is this to handle different versions of nn.functional?

@fkiraly fkiraly assigned PranavBhatP and unassigned pranavvp16 Aug 28, 2025
@fkiraly
Copy link
Collaborator

fkiraly commented Aug 28, 2025

@fkiraly could this be assigned to me? I don't think it will be visible on the kanban board...

apologies, it was assigned to the wrong Pranav. GitHub autocompletes when you start typing @, so if you have very similar GH names it is easy to select unintentionally.

@agobbifbk
Copy link

In the original implementation there is this switch:

what is this switch for, @agobbifbk? Is this to handle different versions of nn.functional?

The function was introduced recently, and probably it is bugged for some version (or some platform). The authors put a switch to be sure that it works in different versions of PT.

@phoeenniixx phoeenniixx moved this from PR in progress to PR under review in May - Sep 2025 mentee projects Aug 29, 2025
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.

lgtm

@fkiraly fkiraly merged commit 21a46a2 into sktime:main Sep 5, 2025
35 checks passed
@github-project-automation github-project-automation bot moved this from PR under review to Done in May - Sep 2025 mentee projects Sep 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

[ENH] Implement SAMFORMER from DSIPTS in PTF v2
4 participants