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

Shape squeeze edge case bug fix #911

Merged
merged 5 commits into from
Jul 9, 2020
Merged

Conversation

benidis
Copy link
Contributor

@benidis benidis commented Jul 7, 2020

Fixes an edge case where we pass a single value and np.squeeze suppresses all dimensions.

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.

Is this edge case covered by tests?

@benidis
Copy link
Contributor Author

benidis commented Jul 8, 2020

Is this edge case covered by tests?

No, I am adding a test.

@@ -87,6 +87,8 @@ class MeanValueImputation(MissingValueImputation):
"""

def __call__(self, values: np.ndarray) -> np.ndarray:
if len(values) == 1:
return DummyValueImputation()(values)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to pass a user specified dummy_value when constructing the DummyValueImputation object here? Essentially, all these classes would allow specifying the dummy_value, to be used in extreme cases.

But maybe not urgent and can be addressed later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so the DummyValueImputation accepts a value that uses to impute. With the current implementation it uses the default. We can always add one more argument to all the imputation methods to override the default but maybe this is too much. If needed we can always include it easily.

@codecov-commenter
Copy link

codecov-commenter commented Jul 8, 2020

Codecov Report

Merging #911 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #911   +/-   ##
=======================================
  Coverage   86.04%   86.05%           
=======================================
  Files         195      195           
  Lines       11869    11877    +8     
=======================================
+ Hits        10213    10221    +8     
  Misses       1656     1656           
Impacted Files Coverage Δ
src/gluonts/transform/feature.py 98.47% <100.00%> (+0.06%) ⬆️

@benidis benidis merged commit 134da35 into awslabs:master Jul 9, 2020
@benidis benidis deleted the shape_bug_fix branch July 9, 2020 10:00
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.

3 participants