Adding an option for target normalization in SVR #1853
Conversation
…arning/ScikitLearn/Neighbors/RadiusNeighborsClassifier.py
Jimmy-INL
changed the title
There was a problem hiding this comment.
Hi @Jimmy-INL, I have a couple of comments for you to consider, please let me know if you have any question.
| @@ -195,5 +192,6 @@ def _localNormalizeData(self,values,names,feat): | |||
| """ | |||
| if not self.info['normalize']: | |||
| self.muAndSigmaFeatures[feat] = (0.0,1.0) | |||
| self.muAndSigmaTargets[self.target[0]] = (0.0,1.0) | |||
There was a problem hiding this comment.
I guess we can move the "if" check to the SupervisedLearning class
| @@ -45,6 +45,7 @@ class SupervisedLearning(BaseInterface): | |||
| # 'boolean', 'integer', 'float' | |||
| qualityEstType = [] # this describe the type of estimator returned known type are 'distance', 'probability'. | |||
| # The values are returned by the self.__confidenceLocal__(Features) | |||
| info = {'normalize':None, 'normalizeTargets':None} | |||
There was a problem hiding this comment.
Let's be consistent here, please add "problemtype" in the "info" dict.
| @@ -264,9 +266,11 @@ def train(self, tdict, indexMap=None): | |||
| # valueToUse can be either a matrix (for who can handle time-dep data) or a vector (for who can not) | |||
| if self.dynamicFeatures: | |||
| featureValues[:, :, cnt] = (valueToUse[:, :]- self.muAndSigmaFeatures[feat][0])/self.muAndSigmaFeatures[feat][1] | |||
| # targetValues[:,cnt] = (targetValues[:]- self.muAndSigmaFeatures[self.target[0]][0])/self.muAndSigmaFeatures[self.target[0]][1] | |||
| if 'normalizeTargets' in self.info.keys() and self.info['normalizeTargets']==True: | ||
| targetValues = (targetValues - self.muAndSigmaTargets[self.target[0]][0])/self.muAndSigmaTargets[self.target[0]][1] |
There was a problem hiding this comment.
I guess this only works on single target, I think you need to expand for multi-target case.
| @@ -280,6 +284,7 @@ def _localNormalizeData(self,values,names,feat): | |||
| @ Out, None | |||
| """ | |||
| self.muAndSigmaFeatures[feat] = mathUtils.normalizationFactors(values[names.index(feat)]) | |||
| self.muAndSigmaTargets[self.target[0]] = mathUtils.normalizationFactors(values[names.index(self.target[0])]) | |||
There was a problem hiding this comment.
Two comments here:
First, the normalization for target is only based on the first target, I guess we want to normalize each target.
Second, I think either we need to rewrite the _localNormalizeData to handle both feature and target correctly, or have separate methods for feature and target.
| # if self.target[0] in self.muAndSigmaFeatures.keys(): | ||
| if ('normalizeTargets' in self.info.keys()) and self.info['normalizeTargets']: | ||
| target.update((x, y * self.muAndSigmaTargets[self.target[0]][1] + self.muAndSigmaTargets[self.target[0]][0]) for x, y in target.items()) |
There was a problem hiding this comment.
Similar comment here, you are only using the first target to do the normalization, I think we need to extend it.
| setting,_ = paramInput.findNodesAndExtractValues(['normalizeTargets']) | ||
| self.info['normalizeTargets'] = setting['normalizeTargets'] |
There was a problem hiding this comment.
I think we can create a method to check if we want to perform normalization or not. For SVR, we can compute the ratio (basically, the normalized parameters and the default parameters), if the normalized parameters are too large, we can provide normalization on the targets.
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
What are the significant changes in functionality due to this change request?
For Change Control Board: Change Request Review
The following review must be completed by an authorized member of the Change Control Board.
<internalParallel>to True.raven/tests/framework/user_guideandraven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.