Repository: fitbenchmarking/fitbenchmarking

Issue:
Review handling of mantid multifit in core fitting routines
**Description of the error**
Looking at the handling of the chi_sq in `fitbenchmarking.core.fitting_benchmarking.loop_over_[minimizers|jacobians|hessians]()` I think this may have been broken at some point.

**Describe the expected result**
These combinations should all be linked to the same datatsets

**Describe the actual result**
I expect that if we run this with multiple jacobians and or hessians we will see each result for these combinations is treated as a new dataset.

**Suggested fix**
The block starting:
```
            for result in results:
                if problem.multifit:
```
Should be moved to the `loop_over_hessians()` function.

**Additional details**#
We should probably expand our testing when fixing this to cover this case.


Discussion:
coveralls: 
[![Coverage Status](https://coveralls.io/builds/44368072/badge)](https://coveralls.io/builds/44368072)

Coverage increased (+0.02%) to 87.759% when pulling **400cc2d38f695fb41bac77e3288e253de2e68ae1 on 920_FixMultiFitForMultiJacobians** into **4b7e835a8db712bf14fc05c7f5ec58cf0c49ecb6 on master**.

AndrewLister-STFC: Also let me know if there's an order you'd rather the regression tests are merged in!
robertapplin: > Also let me know if there's an order you'd rather the regression tests are merged in!

I think it would be better to get #968 merged in first

Patch:
diff --git a/fitbenchmarking/core/fitting_benchmarking.py b/fitbenchmarking/core/fitting_benchmarking.py
index 630c5c88..c2986ae2 100644
--- a/fitbenchmarking/core/fitting_benchmarking.py
+++ b/fitbenchmarking/core/fitting_benchmarking.py
@@ -291,7 +291,6 @@ def loop_over_fitting_software(cost_func, options, start_values_index,
         unselected_minimizers[s] = minimizer_failed
         minimizer_dict[s] = new_minimizer_list
         results.extend(problem_result)
-
     return results, unselected_minimizers, minimizer_dict
 
 
@@ -375,27 +374,11 @@ def loop_over_minimizers(controller, minimizers, options, grabbed_output):
             ########################
             # Loops over Jacobians #
             ########################
-            results, chi_sq, minimizer_list = \
-                loop_over_jacobians(controller,
-                                    options,
-                                    grabbed_output)
-
-            for result in results:
-                if problem.multifit:
-                    # Multi fit
-                    # (will raise TypeError if these are not iterable)
-                    for i in range(len(chi_sq)):
-                        result.update({'dataset_id': i,
-                                       'name': '{}, Dataset {}'.format(
-                                           problem.name, (i + 1))})
-                        individual_result = \
-                            fitbm_result.FittingResult(**result)
-                        results_problem.append(individual_result)
-                else:
-                    # Normal fitting
-                    individual_result = fitbm_result.FittingResult(
-                        **result)
-                    results_problem.append(individual_result)
+            results, minimizer_list = loop_over_jacobians(controller,
+                                                          options,
+                                                          grabbed_output)
+
+            results_problem.extend(results)
             new_minimizer_list.extend(minimizer_list)
 
     return results_problem, minimizer_failed, new_minimizer_list
@@ -412,20 +395,16 @@ def loop_over_jacobians(controller, options, grabbed_output):
     :param grabbed_output: Object that removes third part output from console
     :type grabbed_output: fitbenchmarking.utils.output_grabber.OutputGrabber
 
-    :return: all results as a dictionary of arguments for FittingResult,
-             chi-squared values for each jacobian-hessian pair, and
+    :return: all results as a dictionary of arguments for FittingResult, and
              composite minimizer-jacobian-hessian names.
-    :rtype: list[dict],
-            list[float],
+    :rtype: list[fibenchmarking.utils.fitbm_result.FittingResult],
             list[str]
     """
     cost_func = controller.cost_func
     minimizer = controller.minimizer
     jacobian_list = options.jac_method
     minimizer_name = minimizer
-    jacobian = False
     results = []
-    chi_sq = []
     minimizer_list = []
     minimizer_check = minimizer in controller.jacobian_enabled_solvers
     try:
@@ -455,15 +434,11 @@ def loop_over_jacobians(controller, options, grabbed_output):
                 #######################
                 # Loops over Hessians #
                 #######################
-                new_result, new_chi_sq, new_minimizer_list = \
-                    loop_over_hessians(controller,
-                                       options,
-                                       minimizer_name,
-                                       grabbed_output)
+                new_result, new_minimizer_list = loop_over_hessians(
+                    controller, options, minimizer_name, grabbed_output)
 
                 minimizer_list.extend(new_minimizer_list)
                 results.extend(new_result)
-                chi_sq.extend(new_chi_sq)
                 # For minimizers that do not accept jacobians we raise an
                 # StopIteration exception to exit the loop through the
                 # Jacobians
@@ -472,7 +447,7 @@ def loop_over_jacobians(controller, options, grabbed_output):
     except StopIteration:
         pass
 
-    return results, chi_sq, minimizer_list
+    return results, minimizer_list
 
 
 def loop_over_hessians(controller, options, minimizer_name, grabbed_output):
@@ -488,11 +463,9 @@ def loop_over_hessians(controller, options, minimizer_name, grabbed_output):
     :param grabbed_output: Object that removes third part output from console
     :type grabbed_output: fitbenchmarking.utils.output_grabber.OutputGrabber
 
-    :return: all results as a dictionary of arguments for FittingResult,
-             chi-squared values for each hessian, and
+    :return: all results as a dictionary of arguments for FittingResult, and
              composite minimizer-jacobian-hessian names.
-    :rtype: list[dict],
-            list[float],
+    :rtype: list[fibenchmarking.utils.fitbm_result.FittingResult],
             list[str]
     """
     minimizer = controller.minimizer
@@ -502,7 +475,6 @@ def loop_over_hessians(controller, options, minimizer_name, grabbed_output):
     hessian_list = options.hes_method
     new_result = []
     new_minimizer_list = []
-    new_chi_sq = []
 
     # loop over selected hessian methods
     for hes_method in hessian_list:
@@ -553,15 +525,17 @@ def loop_over_hessians(controller, options, minimizer_name, grabbed_output):
                        'params': controller.final_params,
                        'error_flag': controller.flag,
                        'name': problem.name}
-        new_result.append(result_args)
         new_minimizer_list.append(minimizer_name_hes)
         if problem.multifit:
             # for multifit problems, multiple chi_sq values are stored
-            # in a list, which should extend the empty new_chi_sq list
-            # rather than be appended to it
-            new_chi_sq.extend(chi_sq)
+            # in a list i.e. we have multiple results
+            for i in range(len(chi_sq)):
+                result_args.update(
+                    {'dataset_id': i,
+                     'name': f'{problem.name}, Dataset {i + 1}'})
+                new_result.append(fitbm_result.FittingResult(**result_args))
         else:
-            new_chi_sq.append(chi_sq)
+            new_result.append(fitbm_result.FittingResult(**result_args))
 
         # For minimizers that do not accept hessians we raise an
         # StopIteration exception to exit the loop through the
@@ -569,7 +543,7 @@ def loop_over_hessians(controller, options, minimizer_name, grabbed_output):
         if not minimizer_check:
             break
 
-    return new_result, new_chi_sq, new_minimizer_list
+    return new_result, new_minimizer_list
 
 
 def perform_fit(controller, options, grabbed_output):


Test patch:
diff --git a/fitbenchmarking/core/tests/test_fitting_benchmarking_hessians.py b/fitbenchmarking/core/tests/test_fitting_benchmarking_hessians.py
index 814fedc4..98a64c73 100644
--- a/fitbenchmarking/core/tests/test_fitting_benchmarking_hessians.py
+++ b/fitbenchmarking/core/tests/test_fitting_benchmarking_hessians.py
@@ -115,7 +115,7 @@ class LoopOverHessiansTests(unittest.TestCase):
         self.controller.minimizer = "general"
         new_name = ['general, analytic hessian']
         minimizer_name = "general"
-        _, _, new_minimizer_list = \
+        _, new_minimizer_list = \
             loop_over_hessians(self.controller,
                                self.options,
                                minimizer_name,
@@ -131,7 +131,7 @@ class LoopOverHessiansTests(unittest.TestCase):
         self.controller.minimizer = "deriv_free_algorithm"
         new_name = ['deriv_free_algorithm']
         minimizer_name = "deriv_free_algorithm"
-        _, _, new_minimizer_list = \
+        _, new_minimizer_list = \
             loop_over_hessians(self.controller,
                                self.options,
                                minimizer_name,
@@ -188,10 +188,10 @@ class LoopOverHessiansTests(unittest.TestCase):
         self.controller.parameter_set = 0
 
         self.controller.minimizer = "deriv_free_algorithm"
-        results, _, _ = loop_over_hessians(self.controller, self.options,
-                                           self.controller.minimizer,
-                                           self.grabbed_output)
-        self.assertEqual(results[0]["error_flag"], 6)
+        results, _ = loop_over_hessians(self.controller, self.options,
+                                        self.controller.minimizer,
+                                        self.grabbed_output)
+        self.assertEqual(results[0].error_flag, 6)
 
 
 if __name__ == "__main__":
diff --git a/fitbenchmarking/core/tests/test_fitting_benchmarking_jacobians.py b/fitbenchmarking/core/tests/test_fitting_benchmarking_jacobians.py
index c2a2fdc8..61c5e451 100644
--- a/fitbenchmarking/core/tests/test_fitting_benchmarking_jacobians.py
+++ b/fitbenchmarking/core/tests/test_fitting_benchmarking_jacobians.py
@@ -105,7 +105,7 @@ class LoopOverJacobiansTests(unittest.TestCase):
         Mock function to be used instead of loop_over_hessians
         """
         minimizer_name = args[2]
-        return [], [], [minimizer_name]
+        return [], [minimizer_name]
 
     @patch('{}.loop_over_hessians'.format(FITTING_DIR))
     def test_single_jacobian(self, loop_over_hessians):
@@ -117,7 +117,7 @@ class LoopOverJacobiansTests(unittest.TestCase):
         self.controller.minimizer = "general"
         loop_over_hessians.side_effect = self.mock_func_call
         new_name = ['general: scipy 3-point']
-        _, _, new_minimizer_list = \
+        _, new_minimizer_list = \
             loop_over_jacobians(self.controller,
                                 self.options,
                                 self.grabbed_output)
@@ -133,7 +133,7 @@ class LoopOverJacobiansTests(unittest.TestCase):
         self.controller.minimizer = "general"
         loop_over_hessians.side_effect = self.mock_func_call
         new_name = ['general: scipy 3-point', 'general: scipy 2-point']
-        _, _, new_minimizer_list = \
+        _, new_minimizer_list = \
             loop_over_jacobians(self.controller,
                                 self.options,
                                 self.grabbed_output)
@@ -150,7 +150,7 @@ class LoopOverJacobiansTests(unittest.TestCase):
         self.controller.minimizer = "deriv_free_algorithm"
         loop_over_hessians.side_effect = self.mock_func_call
         new_name = ['deriv_free_algorithm']
-        _, _, new_minimizer_list = \
+        _, new_minimizer_list = \
             loop_over_jacobians(self.controller,
                                 self.options,
                                 self.grabbed_output)
diff --git a/fitbenchmarking/core/tests/test_fitting_benchmarking_minimizers.py b/fitbenchmarking/core/tests/test_fitting_benchmarking_minimizers.py
index fbdbeba4..bacddab4 100644
--- a/fitbenchmarking/core/tests/test_fitting_benchmarking_minimizers.py
+++ b/fitbenchmarking/core/tests/test_fitting_benchmarking_minimizers.py
@@ -98,13 +98,10 @@ class LoopOverMinimizersTests(unittest.TestCase):
         self.grabbed_output = output_grabber.OutputGrabber(self.options)
         self.controller.parameter_set = 0
         self.count = 0
-        self.result_args = {'options': self.options,
-                            'cost_func': self.cost_func,
-                            'jac': "jac",
-                            'hess': 'hess',
-                            'initial_params': self.problem.starting_values[0],
-                            'params': [],
-                            'chi_sq': 1}
+        self.result = fitbm_result.FittingResult(
+            options=self.options, cost_func=self.cost_func, jac="jac",
+            hess="hess", initial_params=self.problem.starting_values[0],
+            params=[], chi_sq=1)
 
     def mock_func_call(self, *args, **kwargs):
         """
@@ -113,7 +110,7 @@ class LoopOverMinimizersTests(unittest.TestCase):
         results = self.results[self.count]
         minimizer_list = self.minimizer_list[self.count]
         self.count += 1
-        return results, self.chi_sq, minimizer_list
+        return results, minimizer_list
 
     def test_run_minimzers_none_selected(self):
         """
@@ -133,8 +130,7 @@ class LoopOverMinimizersTests(unittest.TestCase):
         Tests that some minimizers are selected
         """
         self.options.algorithm_type = ["general"]
-        self.results = [[self.result_args]]
-        self.chi_sq = 1
+        self.results = [[self.result]]
         self.minimizer_list = [["general"]]
         loop_over_hessians.side_effect = self.mock_func_call
 
@@ -151,8 +147,7 @@ class LoopOverMinimizersTests(unittest.TestCase):
         """
         Tests that all minimizers are selected
         """
-        self.results = [[self.result_args], [self.result_args]]
-        self.chi_sq = [1]
+        self.results = [[self.result], [self.result]]
         self.minimizer_list = [["general"], ["deriv_free_algorithm"]]
         loop_over_hessians.side_effect = self.mock_func_call
 
diff --git a/fitbenchmarking/systests/linux_expected_results/multifit.txt b/fitbenchmarking/systests/linux_expected_results/multifit.txt
index 334d77a7..7e51383f 100644
--- a/fitbenchmarking/systests/linux_expected_results/multifit.txt
+++ b/fitbenchmarking/systests/linux_expected_results/multifit.txt
@@ -1,4 +1,4 @@
-                                                      mantid
-                          Levenberg-Marquardt: scipy 2-point
-Basic MultiFit, Dataset 1                           8.16 (1)
-Basic MultiFit, Dataset 2                          15.28 (1)
\ No newline at end of file
+                                                      mantid                                   
+                          Levenberg-Marquardt: scipy 2-point Levenberg-Marquardt: scipy 3-point
+Basic MultiFit, Dataset 1                           8.16 (1)                           8.16 (1)
+Basic MultiFit, Dataset 2                          15.28 (1)                          15.28 (1)
\ No newline at end of file
diff --git a/fitbenchmarking/systests/test_regression.py b/fitbenchmarking/systests/test_regression.py
index 75d8bbc1..6beaf10e 100644
--- a/fitbenchmarking/systests/test_regression.py
+++ b/fitbenchmarking/systests/test_regression.py
@@ -53,7 +53,9 @@ class TestRegressionAll(TestCase):
         """
         problem_sub_directory = "multifit_set"
 
-        run_benchmark(self.results_dir, problem_sub_directory, ["mantid"])
+        run_benchmark(self.results_dir, problem_sub_directory,
+                      override_software=["mantid"],
+                      jac_num_method={"scipy": ["2-point", "3-point"]})
 
         diff, msg = compare_results(problem_sub_directory, "multifit.txt")
         self.assertListEqual([], diff, msg)
@@ -185,13 +187,16 @@ def compare_results(problem_sub_directory: str, result_filename: str) -> list:
     return diff_result(actual, expected)
 
 
-def setup_options(override_software: list = None) -> Options:
+def setup_options(override_software: list = None,
+                  jac_num_method: dict = None) -> Options:
     """
     Setups up options class for system tests
 
     :param override_software: The software to use instead of the
     software determined by the test type.
     :type override_software: list of strings
+    :param jac_num_method: The jacobian methods to use when fitting.
+    :type jac_num_method: dict{str: list[str]}
 
     :return: Fitbenchmarking options file for tests
     :rtype: fitbenchmarking.utils.options.Options
@@ -227,20 +232,25 @@ def setup_options(override_software: list = None) -> Options:
     opts.software = software.get(TEST_TYPE) if override_software is None \
         else override_software
     opts.minimizers = {s: [minimizers[s]] for s in opts.software}
+    if jac_num_method is not None:
+        opts.jac_num_method = jac_num_method
     return opts
 
 
-def create_options_file(override_software: list = None):
+def create_options_file(override_software: list = None,
+                        jac_num_method: dict = None):
     """
     Creates a temporary options file and returns its name.
 
     :param override_software: The software to use instead of the
     software determined by the test type.
     :type override_software: list of strings
+    :param jac_num_method: The jacobian methods to use when fitting.
+    :type jac_num_method: dict{str: list[str]}
     :return: Name of the temporary options file.
     :rtype: str
     """
-    opts = setup_options(override_software)
+    opts = setup_options(override_software, jac_num_method)
     with NamedTemporaryFile(suffix='.ini', mode='w',
                             delete=False) as opt_file:
         opts.write_to_stream(opt_file)
@@ -249,7 +259,8 @@ def create_options_file(override_software: list = None):
 
 
 def run_benchmark(results_dir: str, problem_sub_directory: str,
-                  override_software: list = None) -> None:
+                  override_software: list = None,
+                  jac_num_method: dict = None) -> None:
     """
     Runs a benchmark of the problems in a specific directory
     and places them in the results directory.
@@ -261,8 +272,10 @@ def run_benchmark(results_dir: str, problem_sub_directory: str,
     :param override_software: The software to use instead of the
     software determined by the test type.
     :type override_software: list[str]
+    :param jac_num_method: The jacobian methods to use when fitting.
+    :type jac_num_method: dict{str: list[str]}
     """
-    opt_file_name = create_options_file(override_software)
+    opt_file_name = create_options_file(override_software, jac_num_method)
     problem = os.path.abspath(os.path.join(os.path.dirname(__file__),
                                            os.pardir,
                                            "mock_problems",