Fix rom dataobj #2051
Fix rom dataobj #2051
Conversation
|
Job Mingw Test on 7679bed : invalidated by @joshua-cogliati-inl |
There was a problem hiding this comment.
In addition to the comments I provided inside the code, I have the following comments:
- It is not clear to me how to utilize the DataSet directly as training input, I do not see an example, this may be because I do not familiar with TSA module, could you explain it?
- I do not see updated test or new test to check the proposed modifications. Is it checked in the existing TSA tests?
| else: | ||
| # TODO: The following check may need to be moved to Dummy Class -- wangc 7/30/2018 | ||
| if type(trainingSet).__name__ != 'dict' and trainingSet.type == 'HistorySet': | ||
| if type(trainingSet) != dict and trainingSet.type == 'HistorySet': |
There was a problem hiding this comment.
First, could you add a description to list all possible data structures for trainingSet?
Second, could you add checks for different data structures for trainingSet?
There was a problem hiding this comment.
This looks like a specific check for history set alignment, right? I don't know if we need to find out all the different approaches to ROMs within this PR, do we? This sounds like a bigger issue.
|
|
||
| self._replaceVariablesNamesWithAliasSystem(self.trainingSet, 'inout', False) |
There was a problem hiding this comment.
Could you check to see if this line works with your proposed data structure? In Model.py, this method only accept dict or list as input.
| if self.needsDictTraining: | ||
| self.trainOnDictionary(trainingData, indexMap) | ||
| else: | ||
| self.amITrained = True | ||
| self.muAndSigmaFeatures = dict((f, (0,1)) for f in self.features) |
There was a problem hiding this comment.
These lines is not clear to me. When dataset is needed, I do not see a training process for the ROM. Could you explain it?
There was a problem hiding this comment.
I agree, was a line missed from the old PR? If I recall correctly, we were directly overloading the "train" method if self.needsDictTraining is False.
| @@ -239,15 +255,15 @@ def train(self, tdict, indexMap=None): | |||
| for feat in self.features: | |||
| for index in indexMap.get(feat, []): | |||
| if index not in needFeatures and index not in needTargets: | |||
| needFeatures.append(feat) | |||
| needFeatures.append(index) | |||
There was a problem hiding this comment.
Could you add an explanation here for the change?
dylanjm
force-pushed
the
fix-rom-dataobj
branch
from
7679bed
to
f4edc15
Compare
|
Job Mingw Test on f4edc15 : invalidated by @joshua-cogliati-inl computer rebooted |
| if oldName in sampledVars: | ||
| value = sampledVars.pop(oldName) | ||
| sampledVars[newName] = value | ||
| elif isinstance(sampledVars, list): |
There was a problem hiding this comment.
I realize originalVariables is a deepcopy of sampledVars, but it would be nice if this set of if isinstance checked on the same variable instead of the two different ones.
|
Job Test qsubs sawtooth on 76a9732 : invalidated by @joshua-cogliati-inl timed out in Test Plugins |
Pull Request Description
What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)
#731
What are the significant changes in functionality due to this change request?
Implements changes found in #1718
Allows the option to pass training data sets directly to ROM SupervisedLearning algorithms rather than converting everything to dictionaries.
A flag is used to allow the SVL to self-identify whether it needs legacy training (dictionaries) or can handle training via DataSet.
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.