Skip to content

[ENH] Adding Support of Distribution Loss in v2 Models - #2420

Open
Muhammad-Rebaal wants to merge 15 commits into
sktime:mainfrom
Muhammad-Rebaal:distribution_loss
Open

Muhammad-Rebaal wants to merge 15 commits into
sktime:mainfrom
Muhammad-Rebaal:distribution_loss

Conversation

@Muhammad-Rebaal

@Muhammad-Rebaal Muhammad-Rebaal commented Sep 9, 2026 •

Copy link
Copy Markdown
Member

Fixes #2389

Hi @phoeenniixx,
In order to add support I've done the following changes :

  • Added transform_output in BaseModel to scale predictions back to their real-world numbers.
  • Added output_size in BaseModel to tell the model how many numbers to predict.
  • Added target_normalizer to __init__ in TslibBaseModel to pass the data scaler to the model.

@codecov

codecov Bot commented Sep 9, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.05970% with 8 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@0acb5a0). Learn more about missing BASE report.

Files with missing lines Patch % Lines
...ch_forecasting/models/base/_tslib_base_model_v2.py 25.00% 3 Missing ⚠️
...h_forecasting/models/tide/_tide_dsipts/_tide_v2.py 80.00% 3 Missing ⚠️
pytorch_forecasting/models/base/_base_model_v2.py 94.28% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2420   +/-   ##
=======================================
  Coverage        ?   88.90%           
=======================================
  Files           ?      218           
  Lines           ?    11713           
  Branches        ?        0           
=======================================
  Hits            ?    10414           
  Misses          ?     1299           
  Partials        ?        0           
Flag Coverage Δ
cpu 88.90% <88.05%> (?)
pytest 88.90% <88.05%> (?)

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

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

@Muhammad-Rebaal
Muhammad-Rebaal marked this pull request as ready for review September 10, 2026 13:34

@phoeenniixx phoeenniixx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add some tests here? To see if the shapes are correct etc?

Maybe we can update the test framework even here - to loop over the losses (like in v1). That can happen in a separate PR stacked over this PR.

@Muhammad-Rebaal

Copy link
Copy Markdown
Member Author

Hi @phoeenniixx,
I've added the tests

Comment on lines +146 to +161
def test_transform_output_dict_target_scale():
"""Dict target_scale applies affine denormalization correctly."""
model = _make_model()
raw = torch.randn(4, 12, 1)
center = torch.tensor([10.0, 20.0, 30.0, 40.0])
scale = torch.tensor([2.0, 3.0, 4.0, 5.0])
target_scale = {"center": center, "scale": scale}

result = model.transform_output(raw, target_scale)

assert result.shape == raw.shape
assert torch.allclose(result[0], raw[0] * 2.0 + 10.0)
assert torch.allclose(result[3], raw[3] * 5.0 + 40.0)


def test_transform_output_plain_tensor_target_scale():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are these two tests useful? Why not direclty use the DistributionLoss?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two tests only validate the affine denormalization path (pred * scale + center), which is the straightforward case.
I'll add those distribution loss tests as well

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My doubt is - DistributionLoss does somethign similar no?
(i have not looked at the math of the losses, so i may be wrong)

@Muhammad-Rebaal Muhammad-Rebaal Sep 25, 2026 •

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These 2 tests are checking specifically making sure that Quantile Loss and Point Forecasts (like MAE) get un-scaled correctly, since I made changes in the transform_output function as well.

@phoeenniixx phoeenniixx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are still lacking the tests that check If DistributionLoss works as intended with v2.

Also, this should be present in the test framework? why the base model?

By adding "support" we should also update the models which have this todo, no?

@Muhammad-Rebaal

Muhammad-Rebaal commented Sep 17, 2026 •

Copy link
Copy Markdown
Member Author

I think we are still lacking the tests that check If DistributionLoss works as intended with v2.

Ok I'll add that specific tests as well

Also, this should be present in the test framework? why the base model?

Yeah I got your point up here so that each time it would run it would also check it .

By adding "support" we should also update the models which have this todo, no?

I thought for the model we'd open an umbrella issue that would adding support for each. Although If that's your vision to add in all models I'll update that as well.

@phoeenniixx

Copy link
Copy Markdown
Member

I thought for the model we'd open an umbrella issue that would adding support for each. Although If that's your vision to add in all models I'll update that as well.

We should try with atleast 1-2 models otherwise how would we know if this is working or not?

@Muhammad-Rebaal
Muhammad-Rebaal marked this pull request as draft September 23, 2026 21:40
Comment thread tests/test_models/test_base_model_v2.py Fixed
@Muhammad-Rebaal
Muhammad-Rebaal marked this pull request as ready for review September 25, 2026 15:06
@Muhammad-Rebaal

Copy link
Copy Markdown
Member Author

Hi @phoeenniixx,

I've made these changes :

  • Added support for 2 models TiDe and UniTS
  • Test added in test test_base_model_v2.py because we made changes in base_model_v2.py, test for specific models will auto-detected by the test_all_v2 framework via the NormalDistributionLoss() configs in the pkg files?

"max_prediction_length": self.max_prediction_length,
"min_encoder_length": self._min_encoder_length,
"min_prediction_length": self._min_prediction_length,
"target_normalizer": self.target_normalizer,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we pass a whole object in metadata? is this actually necessary? I think metadata should be lightweight and only should have the "metadata" and not objects unless it is actually 100% necessary

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we changing the forward logic of this model?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a cleaner and more robust way to handles the shapes?

@phoeenniixx phoeenniixx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
This is looking better.
I just have few concerns (pls see above)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ENH] Adding Support of Distribution Loss in v2 Models.

3 participants