Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix scaling for MQ-(C|R)NN when distribution outputs are used #1070

Merged
merged 3 commits into from
Oct 2, 2020
Merged

Fix scaling for MQ-(C|R)NN when distribution outputs are used #1070

merged 3 commits into from
Oct 2, 2020

Conversation

dcmaddix
Copy link
Contributor

@dcmaddix dcmaddix commented Oct 1, 2020 •
edited by lostella
Loading

Issue #, if available:
#1069

Description of changes:

  • Updated default scaling for large to True to be used with DistrOutput
  • Left default for scaling_decoder_dynamic_feature to be False
  • Pass scaling_decoder_dynamic_feature to training and prediction networks
  • Fix the shape of loc and scale

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@dcmaddix dcmaddix requested a review from lostella October 1, 2020 21:19
@lostella
Copy link
Contributor

lostella commented Oct 2, 2020 •
edited
Loading

@dcmaddix thanks! I think scaling must be off when quantile_output is used, because apparently the output doesn’t get scaled back: so you don’t want to scale the input, otherwise for fixed model parameters and two series ts and alpha*ts, you will get the same predictions for any alpha > 0. To allow for scaling in this case, the output quantiles should be scaled back.

On the other hand, scaling appears to be necessary when using distributions, otherwise NaNs are all over the place unless data is nicely scaled (like all time series with the same magnitude or something).

So I ended up making the default value for scaling dependent on what is being used, which maintains backwards compatibility in the quantile_output case, and fixese the distr_output one. Plus I've fixed the shape issue described in #1069.

Edit: I had to bump the test accuracy for the distr_output cases, since now the default scaling behaviour changed.

dcmaddix
dcmaddix commented Oct 2, 2020
View reviewed changes
src/gluonts/model/seq2seq/_forking_network.py
@@ -77,7 +77,7 @@ def __init__(
embedding_dimension: List[int],
distr_output: Optional[DistributionOutput] = None,
quantile_output: Optional[QuantileOutput] = None,
scaling: bool = False,
scaling: bool = True,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we update the default for scaling back to False here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right!

@dcmaddix dcmaddix closed this Oct 2, 2020
@dcmaddix dcmaddix reopened this Oct 2, 2020
@dcmaddix
Copy link
Contributor Author

dcmaddix commented Oct 2, 2020

@dcmaddix thanks! I think scaling must be off when quantile_output is used, because apparently the output doesn’t get scaled back: so you don’t want to scale the input, otherwise for fixed model parameters and two series ts and alpha*ts, you will get the same predictions for any alpha > 0. To allow for scaling in this case, the output quantiles should be scaled back.

On the other hand, scaling appears to be necessary when using distributions, otherwise NaNs are all over the place unless data is nicely scaled (like all time series with the same magnitude or something).

So I ended up making the default value for scaling dependent on what is being used, which maintains backwards compatibility in the quantile_output case, and fixese the distr_output one. Plus I've fixed the shape issue described in #1069.

Edit: I had to bump the test accuracy for the distr_output cases, since now the default scaling behaviour changed.

Looks great, thanks for updating the default to cover both cases! :)

dcmaddix
dcmaddix commented Oct 2, 2020
View reviewed changes
src/gluonts/model/seq2seq/_forking_estimator.py
@@ -154,7 +155,7 @@ def __init__(
enable_encoder_dynamic_feature: bool = True,
enable_decoder_dynamic_feature: bool = True,
trainer: Trainer = Trainer(),
scaling: bool = False,
scaling: Optional[bool] = None,
scaling_decoder_dynamic_feature: bool = False,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want to turn on scaling of the dynamic features or this should be unrelated to the distribution change right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is unrelated maybe, we could do it but maybe as a separate story

@lostella lostella changed the title Setting default for target scaling with MQ-CNN to True to work with D… Fix scaling for MQ-(C|R)NN when distribution outputs are used Oct 2, 2020
@lostella lostella merged commit 48a8b04 into awslabs:master Oct 2, 2020
kashif pushed a commit to kashif/gluon-ts that referenced this pull request Oct 10, 2020
@dcmaddix @lostella @kashif
Fix scaling for MQ-(C|R)NN when distribution outputs are used (awslab…
8de1b0e 
…s#1070)

* Setting default for target scaling with MQ-CNN to True to work with DistrOutput

* fix scaling option default, fix scale shape

* bump test accuracy after changing default scaling behaviour

Co-authored-by: Danielle Robinson <dmmaddix@amazon.com>
Co-authored-by: Lorenzo Stella <stellalo@amazon.com>
@dcmaddix dcmaddix deleted the mqcnn_distr_scale branch October 19, 2020 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lostella lostella lostella approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dcmaddix @lostella