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

Added support for multiple output regression #4080

Merged
merged 1 commit into from Sep 27, 2017

Conversation

Projects
None yet
2 participants
@lilalinux
Copy link

lilalinux commented Sep 15, 2017

What changes were proposed in this pull request?

Added support for multiple output regression. Previously ignored labelIndex is now evaluated for regression, so that left of the index are the inputs and starting from the index (including) up to the end are the outputs

How was this patch tested?

manual tests

Quick checklist

The following checklist helps ensure your PR is complete:

  • Reviewed the Contributing Guidelines and followed the steps within.
  • Created tests for any significant new code additions.
  • Relevant tests for your changes are passing.
  • Ran mvn formatter:format (see formatter instructions for targeting your specific files).

@lilalinux lilalinux changed the title Added support for multiple output regression. Previously ignored labe… Added support for multiple output regression Sep 15, 2017

@AlexDBlack
Copy link
Member

AlexDBlack left a comment

Thanks for this.

The design here is a bit of a difference to how RecordReaderDataSetIterator works, which requires labelIndexFrom/To args. I think it's fine though, but it definitely does need to be documented (at least on the relevant constructors), so users know what to expect and how to use the 'multiple regression targets' case.

Ideally, a simple unit test for this would be great too.

@lilalinux

This comment has been minimized.

Copy link

lilalinux commented Sep 18, 2017

I'll comment the code. However I can't write a unit test, because mvn eclipse:eclipse fails :-/

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Sep 19, 2017

OK, push that up and I'll add a unit test.

@lilalinux lilalinux force-pushed the lilalinux:multiple_output_regression branch from 9f176d0 to 529f34c Sep 21, 2017

@AlexDBlack AlexDBlack self-assigned this Sep 26, 2017

@AlexDBlack

This comment has been minimized.

Copy link
Member

AlexDBlack commented Sep 27, 2017

I'm seeing unit test failures on existing tests (that are confirmed to be passing on master):
image

@lilalinux

This comment has been minimized.

Copy link

lilalinux commented Sep 27, 2017

The old behavior was to ignore numPossibleLabels.
With the new definition if numPossibleLabels is >1, it partitions the data into left side for inputs and right side for outputs.
=> The test should be changed to use numPossibleLabels = -1, 0 or 1 if regression is used and the old behavior is wanted

@AlexDBlack AlexDBlack merged commit 529f34c into deeplearning4j:master Sep 27, 2017

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