-
Notifications
You must be signed in to change notification settings - Fork 709
[ENH] Consistent 3D output for single-target point predictions in TimeXer
v1.
#1936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
PranavBhatP
wants to merge
13
commits into
sktime:main
Choose a base branch
from
PranavBhatP:timexer-output
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
fc93504
alter output format to 3d
PranavBhatP 486ed18
Merge branch 'main' into timexer-output
PranavBhatP 7c21c69
revert unintended removal of assert in test_timexer.py
PranavBhatP 1db93b5
Merge branch 'main' into timexer-output
PranavBhatP 6329b99
remove stepout for handling single and multi-target output and unify …
PranavBhatP e6e0cda
Merge branch 'main' into timexer-output
PranavBhatP 51e912f
add test for expected shape from forward of model
PranavBhatP 8acbcfa
use helper function for checking model forward output shape
PranavBhatP ca78030
Merge branch 'main' into timexer-output
PranavBhatP 7a59443
revert tests for timexer to original version
PranavBhatP 8676044
Merge branch 'main' into timexer-output
PranavBhatP 82a0fad
remove test skip tag for test_integration
PranavBhatP 9ba81f0
add comment to clarify self.n_quantiles=1
PranavBhatP File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please clarify why are we doing
n_quantiles=1
and notn_quantiles = None
here?As from what I have seen so far in the package, we always use
n_quantiles = None
when not usingQuantileLoss
, and makingn_quantiles=1
may change that contract? Although it wont affect the working ig, but for the user this can be misleading?That
QuantileLoss
is being used, even when it is not?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No,
self.n_quantiles
is being set to 1 because in the case of a point prediction, the median quantile is what is being predicted. So instead of settingself.n_quantiles = None
and having step-outs for checking the value ofself.n_quantiles
inside the layer architecture, we simply set it to 1 and let the loss handle the rest.For some context, setting
self.n_quantiles = 1
simplifies how we handle the distinction between a loss likeMAE
andQuantileLoss
. Refer this diff - https://github.com/sktime/pytorch-forecasting/pull/1936/files#diff-8c4bc78319ca93d3c0e93a38a0ee551ac3c4193f717955c0ce3477e4147a9153TLDR; It removes unecessary "if" statements and step-outs in the logic for
TimeXer
's output.Maybe a comment would clarify this in the code, I will add it, thanks for highlighting this.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohh, I see, Thanks!
If this is working, maybe then we should refactor other places as well where this
if/else
block is being used because ofn_quantiles = None
?I jam just thinking it would be good if everything consistent with rest of the API....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just, I am a bit concerned about the meaning here- if a person is using this estimator, they would see
n_quantiles=1
even for point predicitons (I am talking about the user who is just using the estimator and not actually reading the code) and this may confuse the user that "why this param is notNone
like it is for other estimators inptf
?"Also, idts this
if/else
block would be much of a overhead, so, if using this block keeps the user from getting confused, I think we should prefer that.But that is my thought, Please let me know what is your reasoning here...
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reasoning is as simple as this :). Since we are standardising the output format, it makes more sense if we renamed this attribute to
self.output_dim
or something like that, instead ofself.n_quantiles
. That way we have a more generic meaning and assign different values to it only when there is a step-out to a different case (QuantileLoss in our case)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you kindly explain what API or parameter change exactly we are talking about?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in this PR @PranavBhatP is changing the Convention of using
n_quantiles = None
(that happens currently in the package) when the user is not usingQuantileLoss
(@PranavBhatP please correct me if I understood it wrongly), rathern_quantiles=1
is being introduced in absence ofQuantileLoss
. (see comments #1936 (comment) and #1936 (comment)) and I was concerned about the meaning it could convey (see comment #1936 (comment)).He suggested, we could rename the
n_quantiles
tooutput_dim
and it would solve the issue of the meaning. But again this would lead to changes to the code across the whole package. And it would be breaking as a param name is being changed, so as you said, we could deprecate it, but I think we should introduce (if we decide to) to v2 and keep v1 intact, as v1 is already mature enough, and it may not even complete the deprecation cycle, as before that we may completely move to v2. I am assuming the deprecation cycle will complete after 2 realeases, hopefully we might release a full fledged v2 by then. So, I think if we are going to make this change, we should do this in v2.I am still not very sure if this would be a good idea, that's why I wanted if you and @agobbifbk could comment your thoughts on this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me
output_dim
is a good idea, it is more general. I don't think it is necessary to deprecate it if we rename because it is not a parameter that the user can modify, isn't? It just depend on the chosen loss function(s) right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but I am assuming
n_quantiles
are right now used in documentation to define the shape of output when we useQuantileLoss
, so, ig this arg is visible to the user in the documentation? If yes, we might need to deprecate it? I am not sure, what's the process here😅There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a design document is the right process. Write up the vignettes now - and analyze whether
n_quantiles
even appears in them.Then propose alternative vignettes for future use, and from that develop a change and/or deprecation trajectory.