minor changes to twoPointsCrossovers #1723

Jimmy-INL · 2021-11-29T20:24:41Z

Pull Request Description

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

This PR Closes #1724

What are the significant changes in functionality due to this change request?

TwoPointsCrossOver will not error out.

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.

moosebuild · 2021-11-30T00:05:48Z

Job Test qsubs falcon on 1f54de2 : invalidated by @mandd

framework/Optimizers/crossOverOperators/crossovers.py

mandd · 2021-11-30T00:23:05Z

do we have a test that uses this modified twoPointsCrossovers method?

Jimmy-INL · 2021-11-30T00:31:16Z

I used Andrea's test, but I guess this one will take a long time, I can generate a simpler one.

mandd · 2021-12-01T20:18:12Z

framework/Optimizers/crossOverOperators/crossovers.py

@@ -130,22 +129,29 @@ def twoPointsCrossover(parents, parentIndexes,**kwargs):
                              coords={'chromosome': np.arange(int(2*comb(nParents,2))),
                                      'Gene':parents.coords['Gene'].values})
  index = 0
-  for couples in parentIndexes:
+  parentPairs = list(combinations(parents,2))


should we control here the total number of pairs to be considered?

moosebuild · 2021-12-02T16:41:10Z

Job Test qsubs falcon on 7c3deff : invalidated by @Jimmy-INL

moosebuild · 2021-12-02T16:41:35Z

Job Test qsubs sawtooth on 7c3deff : invalidated by @Jimmy-INL

moosebuild · 2021-12-04T00:32:10Z

Job Test qsubs falcon on 7c3deff : invalidated by @Jimmy-INL

moosebuild · 2021-12-04T16:51:36Z

Job Test qsubs falcon on 7c3deff : invalidated by @Jimmy-INL

mandd · 2021-12-06T19:56:32Z

@wangcj05 : this PR is good to me. Can I merge it into devel?

Jimmy-INL · 2021-12-06T20:02:36Z

@mandd @wangcj05. I was wishing that Diego adds the missing unit tests if he has time. I also made other changes to keep the elite parents which seem to help with the tough optimization problem our private citizen has. PR will be posted soon.

wangcj05 · 2021-12-06T20:09:14Z

@mandd Sure, when all the checklists are satisfied, you are free to merge any PR.

mandd · 2021-12-06T20:10:23Z

@Jimmy-INL : let me see if i can do it before the end of today

wangcj05

Hi @Jimmy-INL @mandd,

Thanks for your contributions. I have couple of comments for your consideration. Please let me know if you have any question about my comments.

wangcj05 · 2021-12-07T03:32:22Z

framework/Optimizers/crossOverOperators/crossovers.py

+    if loc1 == loc2:
+      if loc1 == len(locRangeList)-2:
+        locU = loc1
+        locL = loc1 - 2


This indicates the locRangList need to be larger than 4, i.e. locL >= 1 ==> len(locRangeList) >= 5. Is it always true?

I have restructured the method and it should be more clear

wangcj05 · 2021-12-07T03:38:55Z

framework/Optimizers/crossOverOperators/crossovers.py

-      locL=loc2
-      locU=loc1
-    elif loc1<loc2:
+    if loc1 == loc2:


Instead to check the equality here, I would suggest to check the equality of indices directly, i.e., check if index1 == index2? Another suggestion is when index1 == index2, I think you can regenerate index2

see above, note also that I have added a check on the number of genes which should be >=3

wangcj05 · 2021-12-07T03:56:44Z

framework/Optimizers/crossOverOperators/crossovers.py

@@ -130,22 +129,29 @@ def twoPointsCrossover(parents, parentIndexes,**kwargs):
                              coords={'chromosome': np.arange(int(2*comb(nParents,2))),
                                      'Gene':parents.coords['Gene'].values})
  index = 0
-  for couples in parentIndexes:
+  parentPairs = list(combinations(parents,2))


Here, you are using combinations which assume the first dim of parents is the parentIndexes, please add more details in the docstring to make it more clear, i.e., to indicate the first dim is the parent index

Note that parentIndexes is not used anymore, the docstring indicates how parents is supposed to be structured

wangcj05 · 2021-12-07T03:58:21Z

framework/Optimizers/parentSelectors/parentSelectors.py

+        index = int(matrixOperation[2*i,0])
      else:
-        index = int(matrixOperation[i+1,0])
+        index = int(matrixOperation[2*i+1,0])


Could you explain the changes from i to 2i, and i+1 to 2i+1?

this was a bug, we are filling the population 2 children at a time, hence we need to populate it using the 2i and 2i+1 indexes

wangcj05 · 2021-12-07T03:59:41Z

...ork/Optimizers/GeneticAlgorithms/discrete/unconstrained/testGAMaxwoRepTwoPointsCrossover.xml

+<?xml version="1.0" ?>
+<Simulation verbosity="debug">
+  <TestInfo>
+    <name>framework/Optimizers/GA.MaxwoReplacement</name>


change MaxwoReplacement to MaxwoReplacementTwoPointsCrossover

good catch fixed

wangcj05 · 2021-12-07T04:02:09Z

...ork/Optimizers/GeneticAlgorithms/discrete/unconstrained/testGAMaxwoRepTwoPointsCrossover.xml

+      This test assesses the Genetic algorithm using the weighted sum found in myLocalSum.py function.
+      The nominal dimensionality of the test problem is 3.
+      The objective variable is ans. The problem in unconstrained, it is a maximization problem, and the sampling is from discrete variables without replacement.
+      The cross over mechanism used is the twoPointsCrossover algorithm


Could you update the description to reflect the fix proposed in this PR?

The fix in this PR is to make TwoPointsCrossover work, as it wasn't working before, and this ios the first test on it that's why the last line is added. Can you suggest what should I update the description to?

I'm looking for the explanation about failure, and the modifications for the fix @Jimmy-INL

wangcj05 · 2021-12-07T04:03:36Z

...ork/Optimizers/GeneticAlgorithms/discrete/unconstrained/testGAMaxwoRepTwoPointsCrossover.xml

+      <Optimizer class="Optimizers" type="GeneticAlgorithm">GAopt</Optimizer>
+      <SolutionExport class="DataObjects" type="PointSet">opt_export</SolutionExport>
+      <Output class="DataObjects" type="PointSet">optOut</Output>
+      <Output class="OutStreams" type="Print">opt_export</Output>


This line can be removed since you are using IOStep to Print the info.

wangcj05 · 2021-12-07T04:04:31Z

...ork/Optimizers/GeneticAlgorithms/discrete/unconstrained/testGAMaxwoRepTwoPointsCrossover.xml

+
+      <objective>ans</objective>
+      <TargetEvaluation class="DataObjects" type="PointSet">optOut</TargetEvaluation>
+    <Sampler class="Samplers" type="MonteCarlo">MC_samp</Sampler>


increase indent here

wangcj05 · 2021-12-07T04:05:29Z

...ork/Optimizers/GeneticAlgorithms/discrete/unconstrained/testGAMaxwoRepTwoPointsCrossover.xml

+
+    <Models>
+      <ExternalModel ModuleToLoad="../../../../../AnalyticModels/optimizing/myLocalSum.py" name="myLocalSum" subType="">
+        <variables>x1,x2,x3,x4,x5,x6,ans</variables>


Use inputs and outputs instead of variables

wangcj05 · 2021-12-07T04:10:58Z

...icAlgorithms/gold/discrete/unconstrained/MaxwoReplacementTwoPointsCrossover/opt_export_0.csv

+4.0,2.0,1.0,3.0,6.0,5.0,83.0,9.0,20.0,166.0,19.0,accepted,0.0
+4.0,2.0,3.0,5.0,6.0,1.0,73.0,9.0,20.0,146.0,19.0,accepted,0.0
+3.0,1.0,2.0,4.0,5.0,6.0,88.0,9.0,20.0,176.0,19.0,accepted,0.0
+1.0,2.0,3.0,4.0,5.0,6.0,91.0,10.0,20.0,182.0,19.0,final,0.0


couple of questions here:

why the con_objective is not changing?

Will this test converge to the analytical solution? I saw you have marked the test as analytic.

Conv_objective is a boolean that is tro when objective value converges to a user given value which is not the case here that's why it is false. (I think it should be False not zero, but this is the case in most optimizers.)

The analytic solution is 1,2,3,4,5,6 and objective is 91, which is exactly the solution.

I see in the test the target objective value is '-1', does it mean when the objective value converge to '-1', the con_objective will become True? Another question is, in the test, the type of optimization is assigned to 'max' (see below), but why the objective value is '-1'. @Jimmy-INL

When I know that the objective is a positive value and I select conv_obj = -1, this is just to force the algorithm to continue till the iteration limit is reached. I can remove this from this test if it causes confusion.

mandd · 2021-12-07T18:17:50Z

@Jimmy-INL : I have addressed all comments except the one you are discussing with @wangcj05.

moosebuild · 2021-12-08T00:38:10Z

Job Test qsubs falcon on ddc8479 : invalidated by @mandd

moosebuild · 2021-12-08T05:26:51Z

Job Test qsubs falcon on ddc8479 : invalidated by @mandd

moosebuild · 2021-12-08T15:12:58Z

Job Test qsubs falcon on ddc8479 : invalidated by @mandd

wangcj05

additional comment for objective node.

wangcj05 · 2021-12-08T21:03:26Z

...ork/Optimizers/GeneticAlgorithms/discrete/unconstrained/testGAMaxwoRepTwoPointsCrossover.xml

+      <convergence>
+        <objective>-1</objective>
+      </convergence>


I have checked several definitions in the user manual, for GA (see attached picture), it seems to me the value should be always positive, in addition, we should use InputData to check it. @Jimmy-INL @mandd

@wangcj05, that's what I am saying. The algorithm will converge when $Objective = \epsilon^{obj}$. If I don't want to use the objective convergence and want the algorithm to carry on till it reaches the iteration limit, I select $\epsilon^{obj}$ to be something impossible. For this problem, the obj is always positive, so I picked -1 so that conv_obj is always False (0). If I couldn't explain myself, let's discuss this face to face. Thanks.

As discussed, the convergence on objective is not consistent across different optimizations, and it can be confusing, a new issue has been opened #1729

wangcj05

Modifications in this PR are good.

wangcj05 · 2021-12-09T16:28:33Z

PR checklist is satisfied, and PR can be merged into devel

wangcj05 · 2021-12-09T16:29:14Z

@Jimmy-INL @mandd I have approved and merged your PR. Thanks for your contributions.

minor changes to twoPointsCrossovers

1f54de2

Jimmy-INL added defect RAVENv2.2 for RAVENv2.2 Release labels Nov 29, 2021

Jimmy-INL requested a review from mandd November 29, 2021 20:24

Jimmy-INL self-assigned this Nov 29, 2021

mandd reviewed Nov 30, 2021

View reviewed changes

framework/Optimizers/crossOverOperators/crossovers.py Outdated Show resolved Hide resolved

framework/Optimizers/crossOverOperators/crossovers.py Outdated Show resolved Hide resolved

Jimmy-INL added 3 commits November 29, 2021 21:49

Adding a test for the TwoPintsCrossover

612f576

Correcting path in the tests

bb3388e

addressing diego's comment

7c3deff

Jimmy-INL requested a review from mandd November 30, 2021 07:29

mandd reviewed Dec 1, 2021

View reviewed changes

mandd added 6 commits December 6, 2021 15:04

added unit tests

281a359

re-golding file after unit tests mods

6838abd

fixed addition unit test

9de93f1

delete whitespaces

df4e714

edit tests file

d15bcb0

added 2pointXover file

44c04df

wangcj05 requested changes Dec 7, 2021

View reviewed changes

addressing congjian's comments

716b63e

mandd added 2 commits December 7, 2021 10:23

delete whitespaces

d482a04

restored test

eabb785

mandd added 2 commits December 7, 2021 11:24

delete whitespaces

3aca7c4

added new test

ddc8479

wangcj05 requested changes Dec 8, 2021

View reviewed changes

wangcj05 approved these changes Dec 9, 2021

View reviewed changes

wangcj05 merged commit 63d933b into devel Dec 9, 2021

wangcj05 deleted the Jimmy-Fixing-TwoPointsCrossovers branch December 9, 2021 16:28

minor changes to twoPointsCrossovers #1723

minor changes to twoPointsCrossovers #1723

Conversation

Jimmy-INL commented Nov 29, 2021 • edited by wangcj05 Loading

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

moosebuild commented Nov 30, 2021

mandd commented Nov 30, 2021

Jimmy-INL commented Nov 30, 2021

Choose a reason for hiding this comment

moosebuild commented Dec 2, 2021

moosebuild commented Dec 2, 2021

moosebuild commented Dec 4, 2021

moosebuild commented Dec 4, 2021

mandd commented Dec 6, 2021

Jimmy-INL commented Dec 6, 2021

wangcj05 commented Dec 6, 2021

mandd commented Dec 6, 2021 • edited Loading

wangcj05 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandd commented Dec 7, 2021

moosebuild commented Dec 8, 2021

moosebuild commented Dec 8, 2021

moosebuild commented Dec 8, 2021

wangcj05 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangcj05 left a comment

Choose a reason for hiding this comment

wangcj05 commented Dec 9, 2021

wangcj05 commented Dec 9, 2021

Jimmy-INL commented Nov 29, 2021 •

edited by wangcj05

Loading

mandd commented Dec 6, 2021 •

edited

Loading