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

[DOLPHIN-94] Show both training/cross-validation error to check overfiting #95

Merged
merged 3 commits into from
Aug 30, 2015
Merged

[DOLPHIN-94] Show both training/cross-validation error to check overfiting #95

merged 3 commits into from
Aug 30, 2015

Conversation

dongjoon-hyun
Copy link
Contributor

This is the first draft described in #94 .
I will test and amend again after merging #88 .

@dongjoon-hyun
Copy link
Contributor Author

Hi, @beomyeol . Could you review the design of this PR? I think you're the right person. I changed your Validator as ValidatorBase without asking your permission. :)

@beomyeol
Copy link
Contributor

@dongjoon-hyun, I'll take a look at this.

@beomyeol
Copy link
Contributor

@dongjoon-hyun, This looks good. I'm wondering if there is any chance to have difference between training validation and cross validation. If not, we don't need two separate TrainingValidator and CrossValidator classes, right?

@dongjoon-hyun
Copy link
Contributor Author

Ur, I've heard that we need separate classes for REEF parameter injection.
@jsjason , did I misunderstand?

@jsjason
Copy link
Contributor

jsjason commented Aug 28, 2015

@dongjoon-hyun is right, if we're going to use two separate classes. However, for this case I think we should just include stuff the two into a single Validator class. Having a inner abstract class doesn't seem so good.

@dongjoon-hyun
Copy link
Contributor Author

Oh, I see.

@dongjoon-hyun
Copy link
Contributor Author

Hi, @jsjason , @beomyeol . I merged into one. But, as a single Validator class, REEF complains like this again. How can I resolve this with this single Validator class? I thought it's a limitation of REEF. We should have two classes with same logics.

edu.snu.reef.dolphin.neuralnet.NeuralNetworkREEF.main main | REEF job completed: FAILED(org.apache.reef.tang.exceptions.ClassHierarchyException: Repeated constructor parameter detected.  Cannot inject constructor edu.snu.reef.dolphin.neuralnet.NeuralNetworkTask(edu.snu.reef.dolphin.core.DataParser,edu.snu.reef.dolphin.neuralnet.NeuralNetwork,int edu.snu.reef.dolphin.examples.ml.parameters.MaxIterations,edu.snu.reef.dolphin.neuralnet.NeuralNetworkTask$Validator,edu.snu.reef.dolphin.neuralnet.NeuralNetworkTask$Validator))

@beomyeol
Copy link
Contributor

@dongjoon-hyun Oh, now I understand why you used two separate classes. I didn't know two arguments of the same classes cannot be injected. We can make a copy in the constructor of NeuralNetworkTask that needs public copy constructor of Validator. I think this may not look good since it need public copy constructor.
@jsjason @dongjoon-hyun I am okay to go back to have two separate classes If you are okay.

@dongjoon-hyun
Copy link
Contributor Author

Thank you for your quick advice! @beomyeol .

@jsjason . Please give a guide with abstract class. @jsjason .
To avoid the duplication of logic, we need abstract class. But, if abstract class is not permitted, I will make two classes having the same logic.

@jsjason
Copy link
Contributor

jsjason commented Aug 30, 2015

What I suggested was to create a Validator class that checks both training and validation data. It would have methods like getTrainingAccuracy and getValidationAccuracy. Yes, the logic gets repeated but the logic isn't so big, and you can create some private methods if the repetition gets too long.
Another way is to not use Tang injection: new Validator(). Even REEF uses new ...() when it needs more than one instance of the same class.
Yet another way is to declare some enum that separates training data and validation data, and receive it in Validator's methods.

Abstract classes are not preferred because having an abstract class means some classes share the same features, and this means it can be extracted to a separate class. The unshared part can be turned into an interface.

@jsjason
Copy link
Contributor

jsjason commented Aug 30, 2015

You could even do this: declare a class that wraps two Validators, and receive an injection of that wrapper class. The Validators will have to be instantiated as new Validator(...) anyway.

@dongjoon-hyun
Copy link
Contributor Author

Oh, thank you. @jsjason . I will apply the first suggestion!

@jsjason
Copy link
Contributor

jsjason commented Aug 30, 2015

What about the second method of using new Validator()? All methods have their good points.

@dongjoon-hyun
Copy link
Contributor Author

Okay. No problem. Thank you for quick response!

By the way, I have a question.
Escaping Tang means that we cat not store this in the Context in the future right?

@bgchun
Copy link
Contributor

bgchun commented Aug 30, 2015

@dongjoon-hyun NamedObject will solve the issue discussed here; this is under way in REEF-31.

If you prefer to store Validators in Tang, you can use bindVolatile.

@dongjoon-hyun
Copy link
Contributor Author

Thank you for your advice and pointer, @bgchun . REEF-31 looks like the best way. REEF 0.13 will have that? The fix version of REEF-31 is not assigned yet.

@dongjoon-hyun
Copy link
Contributor Author

@jsjason . I hope I changed the code as your advice. Please correct me if I'm wrong.

By the way, the result shows us overfitting status.

~~ nnTask-0 | Iteration: 99
~~ nnTask-0 | Training Error: 0.014999986
~~ nnTask-0 | Cross Validation Error: 0.18
~~ nnTask-0 | # of validation inputs: 100

@bgchun
Copy link
Contributor

bgchun commented Aug 30, 2015

My student's implementing it. I don't think it's ready for 0.13, which we
plan to do feature freeze at the end of September.

On Sun, Aug 30, 2015 at 1:19 PM, Dongjoon Hyun [email protected]
wrote:

Thank you for your advice and pointer, @bgchun https://github.com/bgchun
. REEF-31 looks like the best way. REEF 0.13 will have that? The fix
version of REEF-31 is not assigned yet.


Reply to this email directly or view it on GitHub
#95 (comment).

Byung-Gon Chun

LOG.log(Level.INFO, "# of validation inputs: {0}", String.valueOf(validator.getTotalNum()));
LOG.log(Level.INFO, "Training Error: {0}", String.valueOf(trainingValidator.getError()));
LOG.log(Level.INFO, "Cross Validation Error: {0}", String.valueOf(crossValidator.getError()));
LOG.log(Level.INFO, "# of validation inputs: {0}", String.valueOf(crossValidator.getTotalNum()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use a line that shows us the number of training inputs, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right!

@jsjason
Copy link
Contributor

jsjason commented Aug 30, 2015

@dongjoon-hyun Could the training error be too low because the number of training inputs is too small?

@dongjoon-hyun
Copy link
Contributor Author

Right. The values means that current neural network configuration is much more complex and the data is too small. That's overfitting. But, this PR does not have anything to handle that.

@beomyeol
Copy link
Contributor

@dongjoon-hyun Thank you for your experiment. To avoid overfitting, we can apply various techniques such as weight decaying and dropout later.

@dongjoon-hyun
Copy link
Contributor Author

Sure, @beomyeol . And that's the reason why I'm implementing convolutional layers and pooling layers.

@jsjason
Copy link
Contributor

jsjason commented Aug 30, 2015

@dongjoon-hyun The current changes look good for now. I don't think being unable to store Validators in Contexts would be a big problem yet. It'll depend on our design; if we restrict one network model to exist only in one task and not across several tasks, then it wouldn't make sense to have a Context-level Validator.

@dongjoon-hyun
Copy link
Contributor Author

@jsjason , I totally agree with you.

I slightly worried and tried not to affect on #70 and #73 in Milestone 3.

Anyway, If you don't mind, please merge this~ :)

@jsjason
Copy link
Contributor

jsjason commented Aug 30, 2015

My comment is a bit misleading. I meant that if we assume a certain model replica does not exist across more than one task in a given evaluator, then we wouldn't have to maintain a Context-level Validator. Partitioning a model across several evaluators is a different story.

@dongjoon-hyun
Copy link
Contributor Author

Yep. I'm sure about that. It's my concern due to lack of knowledges. ;)

@jsjason
Copy link
Contributor

jsjason commented Aug 30, 2015

I'm good. @bgchun and @beomyeol, are you okay with the changes too?

@bgchun
Copy link
Contributor

bgchun commented Aug 30, 2015

+1

@beomyeol
Copy link
Contributor

@jsjason I am good, too. Please merge this PR. :)

jsjason added a commit that referenced this pull request Aug 30, 2015
[DOLPHIN-94] Show both training/cross-validation error to check overfiting
@jsjason jsjason merged commit 194900d into snuspl:master Aug 30, 2015
@jsjason jsjason deleted the DOLPHIN-94 branch August 30, 2015 05:07
@dongjoon-hyun
Copy link
Contributor Author

Thank you!

Close #94 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants