[ENH] Consistent 3D output for single-target point predictions in TimeXer v1.

[PranavBhatP]

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Small change in the code to provide a 3d output tensor for point predictions with 3rd dimension set to 1. With this change the output contract for TimeXer is

• Point predictions: (batch_size, predictions, 1) where the 3rd dimension indicates a single target.
• Quantile predicitons: (batch_size, predictions, num_quantiles) where the 3rd dimension indicates the number of quantiles for which the output is generated.

What should a reviewer concentrate their feedback on?

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

[codecov]

Codecov Report

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

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1936   +/-   ##
=======================================
  Coverage        ?   87.07%           
=======================================
  Files           ?      136           
  Lines           ?     8612           
  Branches        ?        0           
=======================================
  Hits            ?     7499           
  Misses          ?     1113           
  Partials        ?        0           

Flag	Coverage Δ
cpu	87.07% <100.00%> (?)
pytest	87.07% <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.

[fkiraly]

I am worried that we are removing tests but not adding tests.

If we are removing an API stepout, then we should remove a stepout in the testes to match, no?

[PranavBhatP]

it was an unintended removal, happened by mistake, sorry. The code stays the same in test_timexer.py

[fkiraly]

ok, so it was a bug! Good that we do reviews.

Related question: is there a stepout we can remove now?

[PranavBhatP]

is there a stepout we can remove now?

with that question in mind, I tried tweaking with a few of the tensor shapes, and I was able to reduce the number of stepouts. now, the default return is a 3d tensor - (batch_size, prediction,_length, n_quantiles). For point predictions, n_quantiles = 1 by default, and for explicit quantiles we equate the n_quantiles=len(loss.quantiles) where loss has to be QuantileLoss

[fkiraly]

You mention you reduced the number of stepouts, but I do not see a change in tests - did you forget to push them?

[PranavBhatP]

no i meant stepouts in the actual model code. We were handling quantile and non-quantile predictions separately inside the model. But with this PR, I have unified the handling to a single format for all types of metrics used (quantile or point).

The code for the tests ultimately remains the same, since we are testing the output shape of predict API for TimeXer, which is not the concern of this PR.

If there is a misunderstanding, please let me know.

[fkiraly]

Yes, I meant tests.

I think thta one of the following must be true:

• there was a stepout in the tests that we can now remove
• the tests were not complete or were faulty (e.g., they are not covering the API this was a stepout from)

[agobbifbk]

Seems that all the shape are well managed. Here you are using the self.n_quantiles as indicator of the number of output per channel. What if I want to use a distribution loss here? Should we rename it in a more general way?

[fkiraly]

Repeating my review request:

• if there was a stepout for the forecaster in tests, please remove the stepout
• if there was no stepout, please review the tests why there was no stepout and/or why they still passed

[PranavBhatP]

Here you are using the self.n_quantiles as indicator of the number of output per channel. What if I want to use a distribution loss here? Should we rename it in a more general way?

I see a possibility of using DistributionLoss metrics with TimeXer, and it might need a few tweaks to get it working. I don't think it works in the current sense. I think we can maybe park this as an item for a separate PR, since we have to again add tests for its usage with distribution losses.

[PranavBhatP]

Repeating my review request:
if there was a stepout for the forecaster in tests, please remove the stepout
if there was no stepout, please review the tests why there was no stepout and/or why they still passed

I looked at the code, and I think there is missing test. Until now we were only testing the output for model.predict. The reason why the tests were passing was because the output of predict remains consistent, independent of the input format as it is dependent on the metric used in the model. However there is nothing to test the output of the model from its forward method. This is pending work and was missed previously, imo. I will add the test in this PR.

[PranavBhatP]

I've added the test for the expected shapes from the forward method of the model, within the _integration method. It's passing all tests.

[phoeenniixx]

Hi, I have a minor doubt about the tests for the output shape:
If you see these lines in _integration:

pytorch-forecasting/pytorch_forecasting/tests/test_all_estimators.py

Lines 326 to 357 in 1956bc8

	raw_predictions = net.predict(
	val_dataloader,
	fast_dev_run=True,
	return_index=True,
	return_decoder_lengths=True,
	mode="raw",
	trainer_kwargs=trainer_kwargs,
	)
	output = raw_predictions.output.prediction
	n_dims = len(output.shape)
	assert n_dims in [2, 3], (
	f"Prediction output must be 2D or 3D, but got {n_dims}D tensor "
	f"with shape {output.shape}"
	)
	if n_dims == 2:
	batch_size, prediction_length = output.shape
	assert batch_size > 0, f"Batch size must be positive, got {batch_size}"
	assert (
	prediction_length > 0
	), f"Prediction length must be positive, got {prediction_length}"
	elif n_dims == 3:
	batch_size, prediction_length, n_features = output.shape
	assert batch_size > 0, f"Batch size must be positive, got {batch_size}"
	assert (
	prediction_length > 0
	), f"Prediction length must be positive, got {prediction_length}"
	assert (
	n_features > 0
	), f"Number of features must be positive, got {n_features}"

They are already testing the forward output (as mode="raw") for a single target and the test for multitarget is still to be added to the Test framework (the tests donot use MultiLoss in configs right now) see #1933

I am confused why do we need to add these tests again?

[fkiraly]

Thanks for adding the new test!

• I would make it a separate test in the class, could you split it?
• I think there should be a function that determines expected output shape. I would do this as follows:
  • give every (non-multitarget) loss a tag loss_dims: int, which tells you the third dimension of the tensor
  • in the _expected_fwd_shape or similar, call that; and if it is multitarget, then iterate over the components

I would also add the test in a separate PR, to be merged before this.

[PranavBhatP]

I am confused why do we need to add these tests again?

I see what you mean. This PR's changes are solely a patch on the existing set of tests specific to test_timexer.py. There is overlap with test_all_estimators.py, so I think it's an open question of whether we need to perform individual model testing on certain aspects when it is already covered by the test_all_estimators.

[PranavBhatP]

I would also add the test in a separate PR, to be merged before this.

So the workflow would be: add the missing test for the old version (in a separate PR) -> replace with tests for the new output format for timexer (in this PR)?

[fkiraly]

I would also add the test in a separate PR, to be merged before this.

So the workflow would be: add the missing test for the old version (in a separate PR) -> replace with tests for the new output format for timexer (in this PR)?

I would say, add the test for the post-1936 state. Then, hopefully, only TimeXer will fail, and you can skip. Merge the test first, and then merge main to 1936, and in this PR you just remove the skip.

[PranavBhatP]

Thanks for adding the new test!
I would make it a separate test in the class, could you split it?
I think there should be a function that determines expected output shape. I would do this as follows:
give every (non-multitarget) loss a tag loss_dims: int, which tells you the third dimension of the tensor
in the _expected_fwd_shape or similar, call that; and if it is multitarget, then iterate over the components
I would also add the test in a separate PR, to be merged before this.

I was going through this change request and trying to implement it with the workflow you just suggested, I don't understand which class you are referring to. I am not editing the tests in test_all_estimators.py if that was what you meant, I am only editing tests in the individual model tests i.e test_timexer.py

[PranavBhatP]

give every (non-multitarget) loss a tag loss_dims: int, which tells you the third dimension of the tensor

is this change relevant to #1936 and #1951? I think its a whole new PR in itself, since it involves changes to the individual metrics class.