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.

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

Mqcnn rts #668

Merged
merged 56 commits into from
May 18, 2020
Merged

Mqcnn rts #668

merged 56 commits into from
May 18, 2020

Conversation

lovvge
Copy link
Contributor

@lovvge lovvge commented Feb 25, 2020

Issue #, if available:

Description of changes:

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

Todo

@lovvge lovvge requested a review from jaheba February 25, 2020 12:06
@codecov-io
Copy link

codecov-io commented Apr 15, 2020

Codecov Report

Merging #668 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #668   +/-   ##
=======================================
  Coverage   85.87%   85.87%           
=======================================
  Files         172      172           
  Lines       11037    11037           
=======================================
  Hits         9478     9478           
  Misses       1559     1559           

@AaronSpieler
Copy link
Contributor

AaronSpieler commented Apr 17, 2020

The option to ignore dynamical-features will need some refactoring in the _forking_network.py.
Basically I effectively added a test also that tests whether this is possible currently, and its not, which is why all tests are failing.

@AaronSpieler
Copy link
Contributor

So from master there was a change that removed shuffling all-together except on a single batch basis.

decoder_input_static, _, decoder_input_dynamic = self.enc2dec(
decoder_input_static, decoder_input_dynamic = self.enc2dec(
Copy link
Contributor

Choose a reason for hiding this comment

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

This must have been a pretty big bug in the seq2seq library right? If you look at the default enc2dec the third second output is the decoder_input_dynamic, not the third, thus decoder_input_dynamic has been dropped thus far and future_dynamic_feat used in its place

Copy link
Contributor

Choose a reason for hiding this comment

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

I also removed the third output of enc2dec because it was never used and I did not see the point, but I'm open to putting that back.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lostella do you have any preference here regarding my last comment?

lostella
lostella previously approved these changes May 14, 2020
Copy link
Contributor

@lostella lostella left a comment

Choose a reason for hiding this comment

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

Looks good to me! What an effort! Any last-minute adjustments? After those this can go in

src/gluonts/block/encoder.py Outdated Show resolved Hide resolved
@AaronSpieler AaronSpieler merged commit 0d963f7 into awslabs:master May 18, 2020
@lostella lostella added the BREAKING This is a breaking change (one of pr required labels) label May 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING This is a breaking change (one of pr required labels)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants