[ENH] Refactor _loss_mapping

[ParamThakkar123]

Reference Issues/PRs

Fixes #1957

What does this implement/fix? Explain your changes.

This refactor moves legacy loss compatibility mappings and tensor shape checks to metric tags in the respective _pkg classes.
Test-specific parameters and dataloader configurations are now provided by a standardized _get_test_dataloaders_from method in each metric package.
All test logic for selecting compatible losses and checking output shapes is now tag-driven, improving maintainability and extensibility.
Legacy mapping code and if/else logic have been removed from the test suite.

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

Any other comments?

PR checklist

• The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
• 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

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[ParamThakkar123]

@fkiraly

[ParamThakkar123]

Converting this to draft PR. Looking for feedback

[phoeenniixx]

Thanks and Welcome to pytorch-forecasting @ParamThakkar123!
I have few questions/suggestions:

• Donot use get_test_train_params for model_pkg as the fixtures come from get_base_test_params

• I suspect somewhere compatible_loss list is returned empty and this is the reason, some models are looking for get_test_train_params (which should not happen for v1).

• Also, can you please update the description of the PR to add the following info:

  • The EXACT changes you are doing to the files
  • Why are you doing it
  • Expected behavior of the test framework after these changes.

  This would make reviewing easy for the reviewers.

[phoeenniixx]

If you have any problems/doubts, please feel free to ping us on discord. You can also join weekly community meet which happens every Friday at 13:00 UTC at the meetups channels on discord.
https://discord.com/invite/54ACzaFsn7

[ParamThakkar123]

Hello @phoeenniixx Thanks for the amazing feedback. And yeah there are some metrics for which no compatible losses are found. So should I skip them in the test loop?

[ParamThakkar123]

Or what kind of changes should I exactly be doing ?

[ParamThakkar123]

A fallback?

[fkiraly]

Ahead of a review - you are reformatting a larger number of files.
Could you kindly do that in a separate pull request, to avoid a large scope?

[ParamThakkar123]

Ahead of a review - you are reformatting a larger number of files. Could you kindly do that in a separate pull request, to avoid a large scope?

Yes. Will do that

[phoeenniixx]

And yeah there are some metrics for which no compatible losses are found.

So, here we use metrics as loss and, these compatible losses are calculated for the models which are being tested. These compatible losses are called in test_all_estimators to pair the compatible losses with the models. So what is happening, imo, is right now get_compatible_losses is not collecting all the losses correctly.

So should I skip them in the test loop?

These were collected before, so it should be possible to collect them with the new changes, so, I think we should not skip them. Rather, please check if there are some bugs

To check if all the losses are being collected correctly, please run tests from TestAllPtForecasters for both v1 and v2.

Ideally, both of these tests should pass and all the losses should be collected correctly.

[codecov]

Codecov Report

❌ Patch coverage is 65.89147% with 44 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@a3bc452). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
...h_forecasting/metrics/base_metrics/_base_object.py	21.42%	11 Missing ⚠️
pytorch_forecasting/tests/_loss_mapping.py	68.42%	6 Missing ⚠️
...ributions_pkg/_beta/_beta_distribution_loss_pkg.py	75.00%	3 Missing ⚠️
...g/_log_normal/_log_normal_distribution_loss_pkg.py	75.00%	3 Missing ⚠️
...ributions_pkg/_mqf2/_mqf2_distribution_loss_pkg.py	70.00%	3 Missing ⚠️
...rmal/_multivariate_normal_distribution_loss_pkg.py	75.00%	3 Missing ⚠️
...nomial/_negative_binomial_distribution_loss_pkg.py	75.00%	3 Missing ⚠️
...implicit_quantile_network_distribution_loss_pkg.py	66.66%	1 Missing ⚠️
...tions_pkg/_normal/_normal_distribution_loss_pkg.py	66.66%	1 Missing ⚠️
...cs/_point_pkg/_cross_entropy/_cross_entropy_pkg.py	66.66%	1 Missing ⚠️
... and 9 more

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1967   +/-   ##
=======================================
  Coverage        ?   87.06%           
=======================================
  Files           ?      158           
  Lines           ?     9408           
  Branches        ?        0           
=======================================
  Hits            ?     8191           
  Misses          ?     1217           
  Partials        ?        0           

Flag	Coverage Δ
cpu	87.06% <65.89%> (?)
pytest	87.06% <65.89%> (?)

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.

[ParamThakkar123]

@phoeenniixx @fkiraly

[ParamThakkar123]

@phoeenniixx @fkiraly All changes implemented.

[phoeenniixx]

Thanks!
Very Nice! I think it is complete, just change the loss_ndims.

[ParamThakkar123]

Sure @phoeenniixx , noted. Making those changes in a minute

[phoeenniixx]

Actually I meant "loss_ndim":"num_quantiles"
I suggested to add loss_ndims to tags in all metrics was because it is a piece of information that can be used to understand the loss and how it affects the output format of the models.
So, what I was trying to say was in QuantileLoss, the loss_ndims is not fixed (like for other losses, you've seen). It depends on the number of quantiles being used. For example, if num_quantiles = 3 with quantiles [0.1, 0.5, 0.9], the loss is calculated separately for each quantile (10%, 50%, and 90%), so loss_ndims would be 3

[ParamThakkar123]

Actually I meant "loss_ndim":"num_quantiles" why I asked to add loss_ndims to tags in all metrics was because it is a piece of information that can be used to understand the loss and how it affects the output format of the models. So, what I was trying to say was in QuantileLoss, the loss_ndims is not fixed (like for other losses, you've seen). It depends on the number of quantiles being used. For example, if num_quantiles = 3 with quantiles [0.1, 0.5, 0.9], the loss is calculated separately for each quantile (10%, 50%, and 90%), so loss_ndims would be 3

Got it. Really sorry for that

[phoeenniixx]

No worries, you are doing great!

[ParamThakkar123]

@phoeenniixx changes done. Can you please review again ? 😅

[fkiraly]

question: is this function used anywhere? If not, did you forget to replace something with it?

[ParamThakkar123]

Yes, actually I added that function just for checking the output shapes, its not used anywhere and I think we can remove it

[fkiraly]

why did you add it? Or was it added by AI for no reason?

[ParamThakkar123]

Yeah It was actually added while I was using copilot but it added it for checking shapes of tensors but it wasn't used anywhere later

[fkiraly]

Nice! Looks almost done!

Change requests:

• use all_objects instead of manually populated registries for retrieval, see above
• I think LOSS_SPECIFIC_PARAMS can now be entirely replaced by method calls - so I would suggest, look where it is imported and do the replacement.
• can you explain if, and if yes where, the following methods are being called: check_loss_output_shape, and _get_test_dataloaders_from, in metrics?

Design questions:

• is clip_target implied by an intrinsic property of the loss, e.g., is it implied by the range of the target? Or is it purely a test parameter? If it is a property of the loss, then we should think how to encode it by tags. If purely a test parameter, then we should move it.
• can you elaborate how the data loaders are now being constructed? There are now private test methods in the metrics, as well as in the models. This feels a big complicated, and how do they interact?

[ParamThakkar123]

• _get_test_dataloaders_from

The check_loss_output_shape is not being used anywhere just added for testing,removing it in the latest push and the _get_test_dataloaders_from is used in all the distribution, quantile and loss packages

[fkiraly]

can you say where precisely the methods are used, please?

[ParamThakkar123]

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_distributions_pkg/_beta/_beta_distribution_loss_pkg.py#L57C24-L57C50

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_distributions_pkg/_implicit_quantile_network/_implicit_quantile_network_distribution_loss_pkg.py#L55

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_distributions_pkg/_log_normal/_log_normal_distribution_loss_pkg.py#L75

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_distributions_pkg/_multivariate_normal/_multivariate_normal_distribution_loss_pkg.py#L53

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_distributions_pkg/_negative_binomial/_negative_binomial_distribution_loss_pkg.py#L57

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_distributions_pkg/_normal/_normal_distribution_loss_pkg.py#L36

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_point_pkg/_cross_entropy/_cross_entropy_pkg.py#L37

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_point_pkg/_mae/_mae_pkg.py#L35

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_point_pkg/_mape/_mape_pkg.py#L37

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_point_pkg/_mase/_mase_pkg.py#L33

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_point_pkg/_poisson/_poisson_loss_pkg.py#L37

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_point_pkg/_rmse/_rmse_pkg.py#L35

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_point_pkg/_smape/_smape_pkg.py#L37

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_point_pkg/_tweedie/_tweedie_loss_pkg.py#L35

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/_quantile_pkg/_quantile_loss_pkg.py#L46

Written here:

https://github.com/ParamThakkar123/pytorch-forecasting/blob/d5765cadf1f7752ddb14668165827f9fd79c84a4/pytorch_forecasting/metrics/base_metrics/_base_object.py#L87

[ParamThakkar123]

@fkiraly After I replaced LOSS_SPECIFIC_PARAMS by loss_params there are errors from data_loader_kwargs but I made changes to those and ideally data_loader_kwargs should be passed to the dl_default_kwargs but they are not being passed. Is it because _get_test_dataloaders_from are differently accessed ?

[ParamThakkar123]

For distribution, point and quantile losses I am access _get_test_dataloaders_from from the base object while for other packages it's completely defined there as a class method. Does that require a refactor too @fkiraly ?

[ParamThakkar123]

@fkiraly should I keep _get_test_dataloaders_from only for the metric pkgs ?

[fkiraly]

I think we need to discuss this, I am a bit lost about the structure of the calls here. Do you have time to join the meeting about pytorch-forecasting on Sep 26, 13 UTC, on the sktime discord?

[ParamThakkar123]

How about 28th of September @fkiraly ?

[fkiraly]

works for me, but all the ptf developers are in the meeting on the 26th - may I recommend to get in touch on discord for scheduling?