[BUG] Align TimeXer v2 endogenous/exogenous usage with tslib metadata#2009
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2009 +/- ##
=======================================
Coverage ? 86.59%
=======================================
Files ? 164
Lines ? 9690
Branches ? 0
=======================================
Hits ? 8391
Misses ? 1299
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
phoeenniixx
left a comment
There was a problem hiding this comment.
Hello @ahmedkansulum!
Welcome to pytorch-forecasting!
I think there has been some issue while committing, I can't see the changes made by you. Can you please have a look at it?
@phoeenniixx Thank you. |
|
Hi @phoeenniixx, I have now re-applied the change in pytorch_forecasting/models/timexer/_timexer_v2.py so that _forecast uses history_target for the endogenous input and history_cont for the exogenous covariates, consistent with the v2 metadata / TslibDataModule design. I also re-ran pytest -k "TimeXer" and pre-commit locally and they pass. Please let me know if anything else needs adjusting. |
phoeenniixx
left a comment
There was a problem hiding this comment.
Thanks @ahmedkansulum !
Although I have a doubt: I thought we needed to remove the duplicate usage of exogeneous_var etc from param set of __init__ and use metadata instead to initialise these vars? Or am I missing something here?
Also FYI @PranavBhatP
PranavBhatP
left a comment
There was a problem hiding this comment.
@ahmedkansulum along with the changes already in this PR, you are also supposed to remove these params from the __init__. Otherwise everything seems fine.
endogenous_vars: Optional[list[str]] = None,
exogenous_vars: Optional[list[str]] = None,|
Thanks for the clarification, that helps. I have now:
I re-ran Please let me know if you would like any further adjustments (e.g. additional docs or changelog updates). |
|
Thank you all for your support. |
Summary
This PR makes the TimeXer v2 implementation consistent with the v2 /
tslibdesign by removing the duplicated configuration of endogenous and exogenous variables insideTimeXer._forecast.Instead of re-selecting series using
self.endogenous_vars/self.exogenous_varson top of thetslibmetadata, the model now relies solely on the tensors provided by the data pipeline (history_targetandhistory_cont).This implements option 1 discussed in #2003 ("not overriding or passing twice").
Fixes #2003
Motivation / Context
In v2, feature configuration is intended to be described by the
metadataproduced byTslibDataModuleand consumed byTslibBaseModel. TimeXer v2 currently has:metadataendogenous_vars/exogenous_varskwargs that are used in_forecastto re-select columns fromhistory_contThis leads to two different places where the endogenous / exogenous split can be defined, which is exactly the concern raised in #2003.
The maintainers confirmed that option 1 is preferred: relying on the metadata / data pipeline only, i.e. not overriding or passing the configuration twice.
What this PR changes
In
pytorch_forecasting/models/timexer/_timexer_v2.py:TimeXer._forecastno longer usesself.endogenous_varsorself.exogenous_varsto re-select columns fromhistory_cont.Instead, the method now follows the v2 convention:
history_targethistory_contConcretely, the previous block:
is replaced by:
The rest of
_forecast(embedding, encoder, head) remains unchanged.API / behaviour notes
endogenous_varsandexogenous_varsarguments are still present inTimeXer.__init__and are stored onself, but they are no longer used in_forecast.TslibDataModule.Tests
On Windows with Python 3.13, I ran:
The failures reported earlier were due to a local Windows permission issue with the default pytest temp directory; pointing
--basetempat a project-local directory resolved that, and the TimeXer tests now run cleanly with the change in place.Notes
_forecastonly.endogenous_vars/exogenous_varskwargs fromTimeXer.__init__, and