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

Added support for multiple output regression #4080

Merged
merged 1 commit into from Sep 27, 2017

Conversation

@lilalinux
Copy link

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
Contributor

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
Author

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
Contributor

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
Contributor

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
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.