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

Fix bugs with Piecewise process output shape #1362

Merged
merged 1 commit into from
Sep 25, 2017
Merged

Conversation

drasmuss
Copy link
Member

Motivation and context:
This fixes two related bugs with the Piecewise process:

  1. Piecewise({1: [1, 1, 1]}) would return 0.0 for times < 1 (rather than [0, 0, 0]).
  2. Piecewise({1: 1}) would return an array with shape () for times < 1 and shape (1,) for times >= 1,
    because ravel upsizes its inputs to at least 1d. This doesn't really produce any errors, but it's
    probably good to be consistent. I thought about adding some logic to check if the output shape
    should be () or (1,), but then figured that it was probably easier/more consistent to just always have
    the output of the process be an array, instead of a scalar.

I wasn't sure whether to add a changelog entry or not. Technically a bugfix, but it comes so closely on the heels of the feature release that it seems unnecessary (anyone looking at the changelog would probably be seeing both things for the first time).

How has this been tested?

Added a new test, see test_shape_out

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Where should a reviewer start?

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • [n/a] I have updated the documentation accordingly.
  • [n/a] I have included a changelog entry.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Copy link
Member

@tbekolay tbekolay left a comment

Choose a reason for hiding this comment

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

LGTM! Agreed that it shouldn't be a new changelog entry; I pushed a commit that lists this PR in the existing changelog entry, though, as I'll do for #1355. If my new commits look good to @drasmuss I'll merge!

@drasmuss
Copy link
Member Author

Looks good, and that makes sense with the changelog entry.

@tbekolay tbekolay merged commit d4807dc into master Sep 25, 2017
@tbekolay tbekolay deleted the piecewise_shape_out branch September 25, 2017 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants