[jira] [Comment Edited] (MATH-1658) ConvergenceChecker should implement OptimizationData

["François Laferrière (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/MATH-1658?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17736428#comment-17736428 ] 

François Laferrière edited comment on MATH-1658 at 6/23/23 10:41 AM:
---------------------------------------------------------------------

Correct. I stubble on this kind of of field initialization order problem a few time and it is quite annoying.

There are other ways to settle ConvergenceChecker:
 # make  protected BaseOptimizer(ConvergenceChecker) deprecated and leave only the default constructor BaseOptimizer(). Then make parseOptimizationData(...) public so that optimizer creation is done in two steps - A - call constructor - B - parse optimization data. This two steps process may be delegated to a factory.
 # rewrite the BaseOptimizer:parseOptimizationData so that it is private and final, using reflexion and bean interface. The idea is to get the class name of each optData (ex. MaxEval) and search a bean accessor with signature set<className>(Class value).

Proposition 1 is very easy to implements.

Proposition 2 is a bit more tedious, because we will have to convert all parseOptimizationData code (in all BaseOptimizer sub classes)  into a bunch of setter.

for instance
   
{code:java}
 protected void parseOptimizationData(OptimizationData... optData) {
        // The existing values (as set by the previous call) are reused if
        // not provided in the argument list.
        for (OptimizationData data : optData) {
            if (data instanceof MaxEval) {
                maxEvaluations = ((MaxEval) data).getMaxEval();
                continue;
            }
            if (data instanceof MaxIter) {
                maxIterations = ((MaxIter) data).getMaxIter();
                continue;
            }
            if (data instanceof ConvergenceChecker) {
                checker = (ConvergenceChecker<PAIR>) data;
                continue;
            }
        }
    }
{code}


is replaced by

   
{code:java}
 protected void setMaxEval(MaxEval optData) {
        maxEvaluations = optData.getMaxEval();
    }
    
    protected void setMaxIter(MaxIter optData) {
        maxEvaluations = optData.getMaxIter();
    }
    
    protected void setConvergenceChecker(ConvergenceChecker<PAIR> optData) {
        checker = optData;
    }

{code}


As noted above, it is tedious because some code has to be touched in a score of classes. On the other hand side, doing so we get rid of all the ugly instanceof and type casts.


was (Author: JIRAUSER299106):
Correct. I stubble on this kind of of field initialization order problem a few time and it is quite annoying.

There are other ways to settle ConvergenceChecker:
 # make  protected BaseOptimizer(ConvergenceChecker) deprecated and leave only the default constructor BaseOptimizer(). Then make parseOptimizationData(...) public so that optimizer creation is done in two steps - A - call constructor - B - parse optimization data. This two steps process may be delegated to a factory.
 # rewrite the BaseOptimizer:parseOptimizationData so that it is private and final, using reflexion and bean interface. The idea is to get the class name of each optData (ex. MaxEval) and search a bean accessor with signature set<className>(Class value).

Proposition 1 is very easy to implements.

Proposition 2 is a bit more tedious, because we will have to convert all parseOptimizationData code (in all BaseOptimizer sub classes)  into a bunch of setter.

for instance
{{    protected void parseOptimizationData(OptimizationData... optData) {
        // The existing values (as set by the previous call) are reused if
        // not provided in the argument list.
        for (OptimizationData data : optData) {
            if (data instanceof MaxEval) {
                maxEvaluations = ((MaxEval) data).getMaxEval();
                continue;
            }
            if (data instanceof MaxIter) {
                maxIterations = ((MaxIter) data).getMaxIter();
                continue;
            }
            if (data instanceof ConvergenceChecker) {
                checker = (ConvergenceChecker<PAIR>) data;
                continue;
            }
        }
    }
}}

is replaced by

{{    protected void setMaxEval(MaxEval optData) {
        maxEvaluations = optData.getMaxEval();
    }
    
    protected void setMaxIter(MaxIter optData) {
        maxEvaluations = optData.getMaxIter();
    }
    
    protected void setConvergenceChecker(ConvergenceChecker<PAIR> optData) {
        checker = optData;
    }
}}

As noted above, it is tedious because some code has to be touched in a score of classes. On the other hand side, doing so we get rid of all the ugly instanceof and type casts.

> ConvergenceChecker should implement OptimizationData
> ----------------------------------------------------
>
>                 Key: MATH-1658
>                 URL: https://issues.apache.org/jira/browse/MATH-1658
>             Project: Commons Math
>          Issue Type: Improvement
>          Components: legacy
>    Affects Versions: 4.0-beta1
>            Reporter: François Laferrière
>            Priority: Minor
>         Attachments: MATH-1658-ConvergenceChecker-implements-OptimizationData.patch
>
>
> It is a bit peculiar that with BaseOptimizer, ConvergenceChecker can be passed only in constructor.
> It should be possible to change ConvergenceChecker at any other time.
> Further it is a bit strange that the constructor do not accept other OptimizationData.
> In my use cases, I need to create an optimizer once with complex OptimizationData and reuse it many time with very few changes (including changing the ConvergenceChecker).
> So, I suggest that
>  * ConvergenceChecker implements OptimizationData.
>  * protected BaseOptimizer(ConvergenceChecker<PAIR> checker) is replaced by protected BaseOptimizer(OptimizationData... optData) 
> Doing so is compatible with existing code and provide much more flexibility.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)