This repository has been archived by the owner. It is now read-only.

add timing stats module #580

Merged
merged 3 commits into from May 15, 2018

Conversation

Projects
None yet
2 participants
@agibsonccc
Copy link
Member

agibsonccc commented May 13, 2018

What changes were proposed in this pull request?

Adds InputStreamInputSplit support for image record reader and IOTiming module.
(Please fill in changes proposed in this fix)

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)

Please review
https://github.com/deeplearning4j/deeplearning4j/blob/master/CONTRIBUTING.md before opening a pull request.

@agibsonccc agibsonccc requested a review from AlexDBlack May 13, 2018

@AlexDBlack
Copy link
Member

AlexDBlack left a comment

Overall looks good. Some minor issues noted.

Only other thing is whether we also have benchmarks for next(int) - i.e., batch loading? Though might not be that different than batchSize * singleRecordTime anyway...

Mat mat = converter.convert(image.getFrame());

if(mat == null) {
return null;

This comment has been minimized.

@AlexDBlack

AlexDBlack May 14, 2018

Member

I'd double check with @saudet here, but that seems like it would cause issues in the next transform?

This comment has been minimized.

@agibsonccc

agibsonccc May 14, 2018

Member

Ahh, I think that might have been an artifact from a previous commit. That being said, what should we do when a transform fails is a broader question I think we should answer here.


<name>datavec-perf</name>
<!-- FIXME change it to the project's website -->
<url>http://www.example.com</url>

This comment has been minimized.

@AlexDBlack

AlexDBlack May 14, 2018

Member

Fix/delete URL

</dependencies>

<build>
<pluginManagement><!-- lock down plugins versions to avoid using Maven defaults (may be moved to parent pom) -->

This comment has been minimized.

@AlexDBlack

AlexDBlack May 14, 2018

Member

Plugin management section not necessary?
And we have versions already specified in parent pom...

This comment has been minimized.

@agibsonccc

agibsonccc May 14, 2018

Member

Addressed. That was just boilerplate I forgot to remove.

agibsonccc added some commits May 14, 2018

@agibsonccc agibsonccc merged commit d7a1953 into master May 15, 2018

1 check failed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details

@agibsonccc agibsonccc deleted the timingstats branch May 15, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.