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

Using pad_to_size function to remove duplicate code in pad_arrays #1047

Merged
merged 9 commits into from
Sep 29, 2020

Conversation

dcmaddix
Copy link
Contributor

@dcmaddix dcmaddix commented Sep 21, 2020

Description of changes:

  • Moved pad_arrays to util.py
  • Used pad_to_size function to remove duplicate code and return before calling nd.pad if no padding is needed.
  • Moved get_dataset to testutil/dummy_datasets
  • Moved test_pad_arrays_axis to test/util.py

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

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.

@dcmaddix it would be nice to keep the code that requires MXNet under the gluonts.mx subpackage, so that it's clear which imports path require MXNet being installed and which ones don't.

I guess pad_arrays could be part of gluonts.mx.utils?

@dcmaddix
Copy link
Contributor Author

@dcmaddix it would be nice to keep the code that requires MXNet under the gluonts.mx subpackage, so that it's clear which imports path require MXNet being installed and which ones don't.

I guess pad_arrays could be part of gluonts.mx.utils?

yes, I moved it back to mx.batchify. Thanks!

lostella
lostella previously approved these changes Sep 28, 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! I guess the number of test cases changed because of this PR, and this makes some other flaky tests fail consistently -- we should robustify those

@lostella lostella merged commit 60f7d39 into awslabs:master Sep 29, 2020
@dcmaddix dcmaddix deleted the pad_arrays branch September 29, 2020 19:05
kashif pushed a commit to kashif/gluon-ts that referenced this pull request Oct 10, 2020
…slabs#1047)

* Using pad_to_size function to remove deuplicate code in pad_arrays and moving to util

* Removed test_pad_arrays_axis from test_variable_length

* Moving pad_arrays back to mx.batchify since it uses mxnet

* Added unused multi_processing test param back to have the same number of tests so that the flaky tests pass

* Fixing black

Co-authored-by: Danielle Robinson <dmmaddix@amazon.com>
Co-authored-by: Lorenzo Stella <lorenzostella@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants