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

Add a keepDim option to point SDIndexes #7392

Merged
merged 5 commits into from Mar 30, 2019

Conversation

Projects
None yet
2 participants
@rnett
Copy link
Contributor

commented Mar 29, 2019

What changes were proposed in this pull request?

Adds a keepDim option to point SDIndexes. If true, the target dimension will be reduced to size 1 instead of removed.

How was this patch tested?

N/A. Get has no unit tests and the changes are very simple.

rnett added some commits Mar 29, 2019

add a keepDim option to point SDIndexes, which if used will only cont…
…ract the dimension to 1 instead of removing it
@AlexDBlack
Copy link
Member

left a comment

This is a good idea, and it looks good, but I'm not inclined to merge this without at least one test.
Granted, we don't have enough SDVariable.get() tests (didn't realize that until I checked now) - but we do have this one at least: https://github.com/deeplearning4j/deeplearning4j/blob/master/nd4j/nd4j-backends/nd4j-tests/src/test/java/org/nd4j/autodiff/samediff/SameDiffTests.java#L2257-L2282

@rnett

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Huh, that didn't show up in my usage search, I will add some more.

rnett added some commits Mar 29, 2019

@rnett

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

My tests are passing in jenkins, I think we're good.

Tests run: 115, Failures: 0, Errors: 0, Skipped: 4, Time elapsed: 0.582 sec - in org.nd4j.autodiff.samediff.SameDiffTests

Something else is failing though, looks like testMaxAlongDimensionBP, which this won't affect.

@rnett

This comment has been minimized.

Copy link
Contributor Author

commented Mar 29, 2019

Partially fixes #7394 too (better array, could still probably use more tests).

@AlexDBlack
Copy link
Member

left a comment

Thanks. I'll keep the test issue open, IMO we should still add a few more here...

@AlexDBlack AlexDBlack merged commit e50ece1 into deeplearning4j:master Mar 30, 2019

1 check failed

continuous-integration/jenkins/pr-head This commit cannot be built
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.