[math] Add new Python tutorial for minimization #12274

lmoneta · 2023-02-09T16:06:15Z

Convert NumericalMinimization.C tutorial to Python

lmoneta · 2023-02-09T17:22:11Z

@phsft-bot build just on ROOT-ubuntu2004/default with flags -DCTEST_TEST_EXCLUDE_NONE=On

phsft-bot · 2023-02-09T17:22:17Z

Starting build on ROOT-ubuntu2004/default with flags -DCTEST_TEST_EXCLUDE_NONE=On
How to customize builds

couet

LGTM, There is no plot to show?

couet · 2023-02-10T08:18:23Z

tutorials/fit/NumericalMinimization.py

+#   Show usage with all the possible minimizers.
+#  Minimize the Rosenbrock function (a 2D -function)
+#  This example is described also in
+#  http://root.cern.ch/drupal/content/numerical-minimization#multidim_minim


Do you really want to refer to the old drupal wb site?

Good point! This link is broken, we need to fix it also in the C++ version of the tutorial.

lmoneta · 2023-02-10T08:19:50Z

No there is no plot to show, this just performing a minimization of a function and it is a translation of the same existing tutorial in C++. We could maybe make a nice plot showing the steps to converge to the minimum

vepadulano

Thanks for adding the tutorial! See some comments before merging

vepadulano · 2023-02-10T09:55:00Z

tutorials/fit/NumericalMinimization.py

+
+import ROOT
+
+def RosenBrock( vecx ):


It would be good to parse the file through a formatter such as autopep8 or black

vepadulano · 2023-02-10T09:55:11Z

tutorials/fit/NumericalMinimization.py

+
+   return 0;
+
+NumericalMinimization()


Missing newline at the end of the file

vepadulano · 2023-02-10T09:56:15Z

tutorials/fit/NumericalMinimization.py

+# create minimizer giving a name and a name (optionally) for the specific algorithm
+#  possible choices are:
+#      minimizerName                  algoName
+#     Minuit                     Migrad, Simplex,Combined,Scan  (default is Migrad)
+#      Minuit2                    Migrad, BFGS, Simplex,Combined,Scan  (default is Migrad)
+#     GSLMultiMin                ConjugateFR, ConjugatePR, BFGS, BFGS2, SteepestDescent
+#     GSLSimAn
+#     Genetic


The formatting here is very weird, it would be nicer to have the choices listed in some sort of bullet list or similar. Also, I believe this is the documentation of the following function. In Python this should be written as a docstring, after the function name

vepadulano · 2023-02-10T10:03:04Z

tutorials/fit/NumericalMinimization.py

+#
+#  \author Lorenzo Moneta
+
+import ROOT


This is a comment valid for the whole file. It would be better to stick to PEP8 as much as possible when writing Python code. For example, naming of variables and functions should follow snake_case and not CamelCase (apart from the calls to ROOT API which uses CamelCase).

vepadulano · 2023-02-10T10:03:26Z

tutorials/fit/NumericalMinimization.py

+
+   minimizer = ROOT.Math.Factory.CreateMinimizer(minimizerName, algoName);
+   if (not minimizer) :
+      print("Error: cannot create minimizer \"",minName,"\". Maybe the required library was not built?")


There are a few problems here. The first is that the minName variable is not defined. I guess you mean minimizerName. Second, I don't think this way of using print is what we want:
Python 2 will print it as a tuple

>>> a = "here" >>> print("This is it: ", a, " --> hello") ('This is it: ', 'here', ' --> hello')

Python 3 will add unnecessary whitespace to the string.

>>> a = "here" >>> print("This is it: ", a, " --> hello") This is it: here --> hello

The more correct way would be using format, which is Python2 and 3 compatible:

print("Error: cannot create minimizer \"{}\". Maybe the required library was not built?".format(minimizerName))

vepadulano · 2023-02-10T10:07:27Z

tutorials/fit/NumericalMinimization.py

+   # set tolerance and other minimizer parameters
+   # one can also use default values


All comments should start with capital letter

vepadulano · 2023-02-10T10:07:57Z

tutorials/fit/NumericalMinimization.py

+
+   f = ROOT.Math.Functor(RosenBrock,2);
+
+   #evaluate function at a point


Comment should start with capital letter. Also, there should be a whitespace after the # sign

vepadulano · 2023-02-10T10:08:11Z

tutorials/fit/NumericalMinimization.py

+   f = ROOT.Math.Functor(RosenBrock,2);
+
+   #evaluate function at a point
+   import numpy as np


import statements should go at the top of the module

vepadulano · 2023-02-10T10:09:24Z

tutorials/fit/NumericalMinimization.py

+   ret = minimizer.Minimize();
+
+   xs = minimizer.X();
+   print("Minimum: f(", xs[0], ",", xs[1],"): ",minimizer.MinValue())


In all print statements please use format, see comment above

vepadulano · 2023-02-10T10:11:15Z

tutorials/fit/NumericalMinimization.py

+             "converged to the right minimum!")
+   else :
+      print("Minimizer " ,minimizerName, " - ",algoName, "failed to converge !!!");
+      ROOT.Error("NumericalMinimization","fail to converge!");


Why do you use ROOT.Error here? In the error case above (the one if (not minimizer) :) you were just returning an error code. The tutorial should be more consistent regarding error handling.

If you decide to keep the error here, it should be a raise RuntimeError or other standard Python exceptions.

lmoneta · 2023-02-10T16:34:58Z

Thank you very much @vepadulano for all the comments. I have included them in the new version.

phsft-bot · 2023-02-10T16:39:15Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

phsft-bot · 2023-02-10T16:39:30Z

Build failed on ROOT-ubuntu2004/python3.
See console output.

vepadulano · 2023-02-13T10:18:30Z

tutorials/fit/NumericalMinimization.py

+import ROOT
+import numpy as np
+
+def RosenBrock( vecx ):


The file is still missing a pass with a formatter such as autopep8

vepadulano · 2023-02-13T10:18:44Z

tutorials/fit/NumericalMinimization.py

+# create minimizer giving a name and a name (optionally) for the specific algorithm
+#  possible choices are:
+#     minimizerName                  algoName
+#
+#     Minuit                     Migrad, Simplex,Combined,Scan  (default is Migrad)
+#     Minuit2                    Migrad, BFGS, Simplex,Combined,Scan  (default is Migrad)
+#     GSLMultiMin                ConjugateFR, ConjugatePR, BFGS, BFGS2, SteepestDescent
+#     GSLSimAn
+#     Genetic


These comments should become a docstring, right?

vepadulano · 2023-02-13T10:20:05Z

tutorials/fit/NumericalMinimization.py

+   ret = minimizer.Minimize();
+
+   xs = minimizer.X();
+   print("Minimum: f(", xs[0], ",", xs[1],"): ",minimizer.MinValue())


This print statement is missing format

Thanks, this is also fixed now!

vepadulano · 2023-02-13T10:20:58Z

tutorials/fit/NumericalMinimization.py

+   if ( ret and minimizer.MinValue()  < 1.E-4 ) :
+      print("Minimizer {} - {} converged to the right minimum!".format(minimizerName,algoName))


I suppose this is the "correct" output from the function right? Is there no correct return value? Does the tutorial only show this print statement? What is the user supposed to see at the end of the tutorial?

The user is supposed to see the minimum XMIN and the minimum value of the function, f(XMIN) and if it fails ret will be false or minimizer.MinValue() will be larger than zero. (the minimum of the function is at zero).

phsft-bot · 2023-02-16T18:01:37Z

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

lmoneta · 2023-02-16T18:12:26Z

@phsft-bot build just on ROOT-performance-centos8-multicore/cxx17 -DCTEST_TEST_EXCLUDE_NONE=On

phsft-bot · 2023-02-16T18:12:32Z

Starting build on ROOT-performance-centos8-multicore/cxx17
How to customize builds

phsft-bot · 2023-02-16T18:43:16Z

Build failed on ROOT-performance-centos8-multicore/cxx17.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Failing tests:

lmoneta requested a review from couet as a code owner February 9, 2023 16:06

lmoneta self-assigned this Feb 9, 2023

lmoneta requested a review from vepadulano February 9, 2023 17:15

lmoneta force-pushed the add_minim_tutorial branch from 0ecf563 to 9efb10b Compare February 9, 2023 17:21

root-project deleted a comment from phsft-bot Feb 9, 2023

couet approved these changes Feb 10, 2023

View reviewed changes

couet reviewed Feb 10, 2023

View reviewed changes

vepadulano requested changes Feb 10, 2023

View reviewed changes

lmoneta force-pushed the add_minim_tutorial branch from 9efb10b to 1d358cf Compare February 10, 2023 16:39

vepadulano requested changes Feb 13, 2023

View reviewed changes

[math] Add new Python tutorial for minimization

8b17151

lmoneta force-pushed the add_minim_tutorial branch from 1d358cf to 8b17151 Compare February 16, 2023 18:01

vepadulano approved these changes Feb 20, 2023

View reviewed changes

lmoneta merged commit 652d7fd into root-project:master Feb 20, 2023

lmoneta deleted the add_minim_tutorial branch February 20, 2023 11:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[math] Add new Python tutorial for minimization #12274

[math] Add new Python tutorial for minimization #12274

lmoneta commented Feb 9, 2023

lmoneta commented Feb 9, 2023

phsft-bot commented Feb 9, 2023

couet left a comment

couet Feb 10, 2023

lmoneta Feb 10, 2023

lmoneta commented Feb 10, 2023

vepadulano left a comment

vepadulano Feb 10, 2023

vepadulano Feb 10, 2023

vepadulano Feb 10, 2023

vepadulano Feb 10, 2023

vepadulano Feb 10, 2023

vepadulano Feb 10, 2023

vepadulano Feb 10, 2023

vepadulano Feb 10, 2023

vepadulano Feb 10, 2023

vepadulano Feb 10, 2023

lmoneta commented Feb 10, 2023

phsft-bot commented Feb 10, 2023

phsft-bot commented Feb 10, 2023

vepadulano Feb 13, 2023

vepadulano Feb 13, 2023

vepadulano Feb 13, 2023

lmoneta Feb 16, 2023

vepadulano Feb 13, 2023

lmoneta Feb 16, 2023

phsft-bot commented Feb 16, 2023

lmoneta commented Feb 16, 2023

phsft-bot commented Feb 16, 2023

phsft-bot commented Feb 16, 2023

		# set tolerance and other minimizer parameters
		# one can also use default values


		f = ROOT.Math.Functor(RosenBrock,2);

		#evaluate function at a point

		if ( ret and minimizer.MinValue() < 1.E-4 ) :
		print("Minimizer {} - {} converged to the right minimum!".format(minimizerName,algoName))


		return 0;

		NumericalMinimization()

[math] Add new Python tutorial for minimization #12274

[math] Add new Python tutorial for minimization #12274

Conversation

lmoneta commented Feb 9, 2023

lmoneta commented Feb 9, 2023

phsft-bot commented Feb 9, 2023

couet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmoneta commented Feb 10, 2023

vepadulano 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

lmoneta commented Feb 10, 2023

phsft-bot commented Feb 10, 2023

phsft-bot commented Feb 10, 2023

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

phsft-bot commented Feb 16, 2023

lmoneta commented Feb 16, 2023

phsft-bot commented Feb 16, 2023

phsft-bot commented Feb 16, 2023

Failing tests: