allow indexer to attach specific manager #1688
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zachgk
reviewed
Jun 2, 2022
| import org.testng.Assert; | ||
| import org.testng.annotations.Test; | ||
|
|
||
| public class ManagerAttachmentTest { |
There was a problem hiding this comment.
Instead of creating these tests for both MXNet engine and PyTorch engine, can you move it into the integration module? That is where we put the tests representing behavior shared for the entire engine spec. Engines that don't fully implement the spec can return an UnsupportedOperationException and it will not be considered failing the test
| * @param index the section of this {@code NDArray} to return | ||
| * @return the partial {@code NDArray} | ||
| */ | ||
| default NDArray get(NDIndex index, NDManager manager) { |
There was a problem hiding this comment.
Minor thing, but I feel like it would be more consistent to put manager as the first argument
1. moved MXNet and PyTorch unit test into integration test module 2. Fix TensorFlow tempAttach issue Change-Id: I213b34caf51679a5ae7d955414c4bc5c4db91c1a
Change-Id: Iaca16533bfdb7611a4d68d574b5a7fa884280bc0
frankfliu
approved these changes
Jun 2, 2022
|
Rectifications look great @zachgk @frankfliu |
zachgk
approved these changes
Jun 3, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This feature allows to pass an NDManager to NDArray indexation operation so that the result is attached to the passed NDManager instead of to the original NDManager of the NDArray.
Thus, ArrayDataset.get() does not need to attach the result of the indexation a second time, thus batch creation is speeded up.
In my performance tests, I got following improvement:
Before:
It took 6872ms totally
It took 6876ms totally
It took 6825ms totally
It took 6923ms totally
After:
It took 6734ms totally
It took 6709ms totally
It took 6728ms totally
It took 6703ms totally
So the training time is reduced by 2.2% !
Also, this pull request fixes a bug that temporary resources are returned to the wrong manager.