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

Mandd/mutator changes #1968

Merged
merged 53 commits into from
Nov 23, 2022
Merged

Mandd/mutator changes #1968

merged 53 commits into from
Nov 23, 2022

Conversation

mandd
Copy link
Collaborator

@mandd mandd commented Sep 15, 2022


Pull Request Description

What issue does this change request address? (Use "#" before the issue to link it, i.e., #42.)

Close #1953
close #1959
close #1976

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.

  • 1. Review all computer code.
  • 2. If any changes occur to the input syntax, there must be an accompanying change to the user manual and xsd schema. If the input syntax change deprecates existing input files, a conversion script needs to be added (see Conversion Scripts).
  • 3. Make sure the Python code and commenting standards are respected (camelBack, etc.) - See on the wiki for details.
  • 4. Automated Tests should pass, including run_tests, pylint, manual building and xsd tests. If there are changes to Simulation.py or JobHandler.py the qsub tests must pass.
  • 5. If significant functionality is added, there must be tests added to check this. Tests should cover all possible options. Multiple short tests are preferred over one large test. If new development on the internal JobHandler parallel system is performed, a cluster test must be added setting, in XML block, the node <internalParallel> to True.
  • 6. If the change modifies or adds a requirement or a requirement based test case, the Change Control Board's Chair or designee also needs to approve the change. The requirements and the requirements test shall be in sync.
  • 7. The merge request must reference an issue. If the issue is closed, the issue close checklist shall be done.
  • 8. If an analytic test is changed/added is the the analytic documentation updated/added?
  • 9. If any test used as a basis for documentation examples (currently found in raven/tests/framework/user_guide and raven/docs/workshop) have been changed, the associated documentation must be reviewed and assured the text matches the example.

@Jimmy-INL Jimmy-INL mentioned this pull request Nov 15, 2022
9 tasks
@Jimmy-INL Jimmy-INL changed the title [WIP] Mandd/mutator changes Mandd/mutator changes Nov 15, 2022
@moosebuild
Copy link

Job Mingw Test on b005a9b : invalidated by @mandd

Copy link
Collaborator Author

@mandd mandd left a comment

Choose a reason for hiding this comment

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

@wangcj05 : this PR is ready to be reviewed

@wangcj05
Copy link
Collaborator

@mandd The following test failed, I guess you need to re-gold the outputs:

FAILED:
Diff tests/framework/Optimizers/GeneticAlgorithms/GAwithEnsembleModel

Comment on lines 490 to 510
'''
if output.name not in optionalOutputNames:
if output.name not in targetEvaluationNames.keys():
if 'batchMode' not in joinedResponse.keys():
output.addRealization(joinedResponse)
else:
inputVarValues = joinedResponse['batchInfo'][0]['batchRealizations'][0]['SampledVars']
for key in inputVarValues.keys():
inputVarValues[key] = np.array([inputVarValues[key]])
metaValues = joinedResponse['batchInfo'][0]['batchRealizations'][0]
for key in metaValues.keys():
metaValues[key] = np.array([metaValues[key]])
outputData = joinedResponse
batchData = {**inputVarValues, **metaValues, **outputData}
output.addRealization(batchData)
else:
output.addRealization(outcomes[targetEvaluationNames[output.name]]['response'])
else:
# collect optional output if present and not already collected
output.addRealization(optionalOutputs[optionalOutputNames[output.name]])
'''
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the commented lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed!

@mandd
Copy link
Collaborator Author

mandd commented Nov 21, 2022

@mandd The following test failed, I guess you need to re-gold the outputs:

FAILED: Diff tests/framework/Optimizers/GeneticAlgorithms/GAwithEnsembleModel

Test has been regolded

@moosebuild
Copy link

Job Test CentOS 8 on 2b3d0db : invalidated by @joshua-cogliati-inl

mystery failure

@moosebuild
Copy link

Job Test Ubuntu 20-2 Optional on 2b3d0db : invalidated by @joshua-cogliati-inl

mystery failure

@moosebuild
Copy link

Job Test Ubuntu 18-2 Python 3 on 2b3d0db : invalidated by @joshua-cogliati-inl

mystery failure

@moosebuild
Copy link

Job Test Fedora 31 on 2b3d0db : invalidated by @joshua-cogliati-inl

mystery failure

@moosebuild
Copy link

Job Test Fedora 32 on 2b3d0db : invalidated by @joshua-cogliati-inl

mystery failure

@moosebuild
Copy link

Job Test Ubuntu 16 on 2b3d0db : invalidated by @joshua-cogliati-inl

mystery failure

Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

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

@mandd I have several comments for you to consider.

@@ -476,6 +476,7 @@ def collectOutput(self,finishedJob,output):
if output.name not in targetEvaluationNames.keys():
# in the event a batch is run, the evaluations will be a dict as {'RAVEN_isBatch':True, 'realizations': [...]}
if isinstance(evaluation,dict) and evaluation.get('RAVEN_isBatch',False):

Copy link
Collaborator

Choose a reason for hiding this comment

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

no need to have an empty space here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

Comment on lines 79 to 84
if kwargs['locs'] == None:
locs = list(set(randomUtils.randomChoice(list(np.arange(offSprings.data.shape[1])),size=2,replace=False)))
locs.sort()
else:
locs = [kwargs['locs'][0], kwargs['locs'][1]]
locs.sort()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Similar lines have been repeated three times in this file, I would to recommend to create a function to perform this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

function locationsGenerator has been added

Comment on lines 42 to 48
if kwargs['locs'] == None:
locs = list(set(randomUtils.randomChoice(list(np.arange(offSprings.data.shape[1])),size=2,replace=False)))
loc1 = locs[0]
loc2 = locs[1]
loc1 = np.minimum(locs[0], locs[1])
loc2 = np.maximum(locs[0], locs[1])
else:
loc1 = kwargs['locs'][0]
loc2 = kwargs['locs'][1]
loc1 = np.minimum(kwargs['locs'][0], kwargs['locs'][1])
loc2 = np.maximum(kwargs['locs'][0], kwargs['locs'][1])
Copy link
Collaborator

Choose a reason for hiding this comment

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

These have been repeated several times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed with new method

This method is designed to randomly mutate a single gene in each chromosome with probability = mutationProb.
@ In, offSprings, xr.DataArray, children resulting from the crossover process
@ In, kwargs, dict, dictionary of parameters for this mutation method:
mutationProb, float, probability that governs the mutation process, i.e., if prob < random number, then the mutation will occur
Copy link
Collaborator

Choose a reason for hiding this comment

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

docstring for "distDict"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

Comment on lines 170 to 176
if kwargs['locs'] == None:
locs = list(set(randomUtils.randomChoice(list(np.arange(offSprings.data.shape[1])),size=2,replace=False)))
locL = np.minimum(locs[0], locs[1])
locU = np.maximum(locs[0], locs[1])
else:
locL = np.minimum(kwargs['locs'][0], kwargs['locs'][1])
locU = np.maximum(kwargs['locs'][0], kwargs['locs'][1])
Copy link
Collaborator

Choose a reason for hiding this comment

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

These lines have been repeated several times.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

new function has been created

@@ -186,6 +198,7 @@ def inversionMutator(offSprings, distDict, **kwargs):
__mutators['scrambleMutator'] = scrambleMutator
__mutators['bitFlipMutator'] = bitFlipMutator
__mutators['inversionMutator'] = inversionMutator
__mutators['randomMutator'] = randomMutator
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please update the manual for the randomMutator

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, added

Comment on lines +42 to +71
def checkSameDataArrays(comment, resultedDA, expectedDA, update=True):
"""
This method compares two identical things
@ In, comment, string, a comment printed out if it fails
@ In, resultedDA, xr.DataArray, the resulted DataArray to be tested
@ In, expectedDA, xr.DataArray, the expected DataArray
@ In, update, bool, optional, if False then don't update results counter
@ Out, res, bool, True if same
"""
res = resultedDA.identical(expectedDA)
if update:
if res:
results["pass"] += 1
else:
print("checking string", comment, '|', resultedDA, "!=", expectedDA)
results["fail"] += 1
return res

results = {'pass': 0, 'fail': 0}

def createElement(tag,attrib={},text={}):
"""
Method to create a dummy xml element readable by the distribution classes
@ In, tag, string, the node tag
@ In, attrib, dict, optional, the attribute of the xml node
@ In, text, dict, optional, the dict containing what should be in the xml text
"""
element = ET.Element(tag,attrib)
element.text = text
return element
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not related to this PR. I think we should start to standardize our unit tests. The first thing to do is to create utils functions for unit tests. For examples, the above functions have been existed in a lot of unit test files.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, it looks like it's a Wednesday morning topics

Copy link
Collaborator Author

@mandd mandd left a comment

Choose a reason for hiding this comment

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

Addressing Wang's comments

@@ -476,6 +476,7 @@ def collectOutput(self,finishedJob,output):
if output.name not in targetEvaluationNames.keys():
# in the event a batch is run, the evaluations will be a dict as {'RAVEN_isBatch':True, 'realizations': [...]}
if isinstance(evaluation,dict) and evaluation.get('RAVEN_isBatch',False):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

This method is designed to randomly mutate a single gene in each chromosome with probability = mutationProb.
@ In, offSprings, xr.DataArray, children resulting from the crossover process
@ In, kwargs, dict, dictionary of parameters for this mutation method:
mutationProb, float, probability that governs the mutation process, i.e., if prob < random number, then the mutation will occur
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added

Comment on lines +42 to +71
def checkSameDataArrays(comment, resultedDA, expectedDA, update=True):
"""
This method compares two identical things
@ In, comment, string, a comment printed out if it fails
@ In, resultedDA, xr.DataArray, the resulted DataArray to be tested
@ In, expectedDA, xr.DataArray, the expected DataArray
@ In, update, bool, optional, if False then don't update results counter
@ Out, res, bool, True if same
"""
res = resultedDA.identical(expectedDA)
if update:
if res:
results["pass"] += 1
else:
print("checking string", comment, '|', resultedDA, "!=", expectedDA)
results["fail"] += 1
return res

results = {'pass': 0, 'fail': 0}

def createElement(tag,attrib={},text={}):
"""
Method to create a dummy xml element readable by the distribution classes
@ In, tag, string, the node tag
@ In, attrib, dict, optional, the attribute of the xml node
@ In, text, dict, optional, the dict containing what should be in the xml text
"""
element = ET.Element(tag,attrib)
element.text = text
return element
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, it looks like it's a Wednesday morning topics

@@ -186,6 +198,7 @@ def inversionMutator(offSprings, distDict, **kwargs):
__mutators['scrambleMutator'] = scrambleMutator
__mutators['bitFlipMutator'] = bitFlipMutator
__mutators['inversionMutator'] = inversionMutator
__mutators['randomMutator'] = randomMutator
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good catch, added

Comment on lines 170 to 176
if kwargs['locs'] == None:
locs = list(set(randomUtils.randomChoice(list(np.arange(offSprings.data.shape[1])),size=2,replace=False)))
locL = np.minimum(locs[0], locs[1])
locU = np.maximum(locs[0], locs[1])
else:
locL = np.minimum(kwargs['locs'][0], kwargs['locs'][1])
locU = np.maximum(kwargs['locs'][0], kwargs['locs'][1])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

new function has been created

Comment on lines 79 to 84
if kwargs['locs'] == None:
locs = list(set(randomUtils.randomChoice(list(np.arange(offSprings.data.shape[1])),size=2,replace=False)))
locs.sort()
else:
locs = [kwargs['locs'][0], kwargs['locs'][1]]
locs.sort()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

function locationsGenerator has been added

Comment on lines 42 to 48
if kwargs['locs'] == None:
locs = list(set(randomUtils.randomChoice(list(np.arange(offSprings.data.shape[1])),size=2,replace=False)))
loc1 = locs[0]
loc2 = locs[1]
loc1 = np.minimum(locs[0], locs[1])
loc2 = np.maximum(locs[0], locs[1])
else:
loc1 = kwargs['locs'][0]
loc2 = kwargs['locs'][1]
loc1 = np.minimum(kwargs['locs'][0], kwargs['locs'][1])
loc2 = np.maximum(kwargs['locs'][0], kwargs['locs'][1])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed with new method

Copy link
Collaborator

@wangcj05 wangcj05 left a comment

Choose a reason for hiding this comment

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

changes look good.

@moosebuild
Copy link

Job Test qsubs sawtooth on cf931e9 : invalidated by @mandd

@wangcj05
Copy link
Collaborator

Test are green, checklist is good. PR can be merged.

@wangcj05 wangcj05 merged commit 668e9b8 into devel Nov 23, 2022
@wangcj05 wangcj05 deleted the mandd/mutatorChanges branch November 23, 2022 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants