Improve test_finite_difference and test_fit_result #289

antonykamp · 2019-12-25T10:47:40Z

Hey guys,

I worked on test_finite_difference and test_fit_result so that they correspond more to the pytest style.

Some words to 'test_fit_result.py': It's not possible to use a fixture in Parameters, that's why I used a dict to store an call the results.
I didn't touch the last test, because the converted version would be more confusing than helpful.

Merry Christmas and a Happy New Year

Update Local Repo

antonykamp · 2019-12-25T11:05:13Z

The test test_constrained/test_constrained_dependent_on_matrixmodel didn't fail because of this PR. If I clone the master branch, the test doesn't work either.

pckroon

Happy holidays :)

I have some comments for you on test_finite_difference so far. I'm not completely happy with test_fit_result either, but I don't quite see how it should be done. Could you look up how Pytest deals with fixtures that return "multiple" items? That way all the *_result fixtures could be condensed into one. Maybe you can do that by doing something like:

@pytest.mark.parametrize('..., minimizer',
    [
        (..., MINPACK),
        (..., [BFGS, NelderMead),
        (..., BasinHopping),
        ...
    ]
)
@pytest.fixture()
def fit_result(...)

In addition, do we really need all the Parameter/Variable fixtures? Maybe we do, I'm not sure. If so, we can consider creating an alphabet in conf.py. which pytest will magically import.

tests/test_finite_difference.py

antonykamp · 2019-12-27T20:03:52Z

I could compress the fixtures to three: parameters a, b and the result_dict. In this case, all the results would be built in a single fixture, which returns a dict with all of them.
The parameters a and b are used the test-method test_fitting.

By the way, I inquire about Fixtures with several return-items.

antonykamp · 2019-12-27T21:05:27Z

I stick to the solution with the dictionary after long research :D

pckroon · 2019-12-28T14:05:29Z

I stick to the solution with the dictionary after long research :D

Could you share some of your considerations? That way we can have a short discussion about it here, without me also having to do the research.

antonykamp · 2019-12-29T10:17:18Z

Just for clearness: You want to create a parametrized fixture. The params of the fixture are the different results of the models. Further, the fixture will be applied to different tests. But now it is difficult to create tests, with each model having a different comparison partner. In this case, there should be a new parallel parameterization. Besides, models often have to be left out (or only one model is tested) and in my eyes, you quickly lose the overview. The tests are not only made for TravisCI, but also for people who need to understand :D

In my eyes, the dictionary allows models to be added speedy, tests to be implemented easily and clearly, and bugs to be viewed quickly.

pckroon · 2019-12-29T19:58:47Z

Very clear, thanks for the write-up. To sum up: the tests are too diverse? Feel free to reorganise/change the tests so that they become more uniform, if you think that makes sense.

As for the tests in finite_difference, we'll need to find a way to use models in pytest.mark.parametrize without making a million different fixtures. (e.g. @pytest.mark.parametrize('model', [modelA, modelB, modelC, ...])). I don't quite know how to do this yet, and I'm hoping you have a good idea :) We'll need it for the rest of the tests anyway.

antonykamp · 2019-12-29T22:09:47Z

On the one hand it's difficult to uniform the tests. For example, only two results are using constraints. We have to separate them. Also chained_result steps often out of line.

On the other hand the tests can be more uniformed than I initially thought. In some cases, a model didn't mention even though it would be passed.

EDIT: I thought a long time, that it's not possible to put fixtures in a parametrizations, because it was a feature request some years ago, but It didn't realise that an offshoot of Pytest can do it. Now I would have with pytest-cases some new possibilities, for example to replace the dict. However, it is not possible with pure pytest...

pckroon · 2019-12-30T15:00:42Z

I never said it would be easy :)
For constraints: you might be able to parametrize them as [a==b] and use [] for no constraints. Or just None.
ChainedMinimizers and their results are a bit magic, so I can see those becoming a separate case.
And let's stick to pure pytest for now and see what we can cook up. It may be an idea to create an alphabet of parameters (a-e) / variables (x-z) in the global namespace, and use those to make models for parametrized tests?

antonykamp · 2019-12-30T17:58:07Z

It's never easy ;-) Some ideas...

I saw that if a model was initiated with None constraints than it gets an empty list. We could use it, because the test, which checks the constraints, uses a for-each loop.
I just want to know if I understood it right: models with ``ChainedMinimizeruseLeastSquares `as objective, right?
It's not beautiful, but we could test the minimizers like this without leaving chained_result out.

assert isinstance(result_name.minimizer, result_minimizer)
if isinstance(result_name.minimizer, ChainedMinimizer):
    for minimizer, cls is zip(result_name.minimizer.minimizers, [BFGS, NelderMead]):
        assert isinstance(minimizer, cls)

(Maybe we could connect the assert statement with the if statement)

pckroon · 2019-12-31T13:58:25Z

I saw that if a model was initiated with None constraints than it gets an empty list. We could use it, because the test, which checks the constraints, uses a for-each loop.

Sure. You could even do a if constraints: ..., which will be False for both None and an empty list.

I just want to know if I understood it right: models with ChainedMinimizer use LeastSquares as objective, right?

Not necessarily. Maybe in the current tests, but it's not generally true.

It's not beautiful, but we could test the minimizers like this without leaving chained_result out.

I then prefer kicking all chained minimizer tests to either a separate test, or even a separate file. As you said earlier: making the tests understandable is at least as important.

antonykamp · 2019-12-31T15:16:04Z

Allright....Then I have ean idea of the new test_fit_result file :)

But I won't work on it until next year :P

Guten Rutsch :)

antonykamp · 2020-06-14T16:05:26Z

Hey,
I'm so sorry I was away for so long. Papers, courses and exams stood between the pull request and me. Now I have no such big projects until October. Perhaps this conversion will be done by then (at least that's my goal).

Does it make sense to continue working on this PR, or have there been many new tests in the meantime?

pckroon · 2020-06-15T09:26:36Z

Heyhey,

it's been quiet here as well. If you want to pick this up again then by all means go ahead.
Just to make sure, merge master first, but I don't expect any changes.

antonykamp · 2020-06-15T13:03:58Z

Bevor I start, I want to sum up the requested changes:

test_finite_difference
Parametrizing fixtures should be applied to the method test_model. The problem was the number of required fixtures for the variables and parameters. A solution could be an alphabet in conf.py.

test_fit_result
The goal is one big method were the six models will be tested. Problems were the missing feature by pytest and diversity of the models.

So far correct?

pckroon · 2020-06-15T13:17:45Z

I think so, yes.
Start with the simpler one (test_finite_difference) before diving into the other.
Maybe something like this would work:

@pytest.mark.parametrize("model, expected_values, initial_values, xdata, noise", (
    [a*x**2 + b*x + c, {'a': 1, 'b': 2, 'c': 3}, {'a': 0, 'b': 0, 'c': 0}, {'x': np.arange(10)}, 0.05]

)
def test_something(model, expected_values, initial_values, xdata, noise):
    if not initial_values:
        initial_values = expected_values.copy()
    y_data = model(**xdata, **expected_values)
    y_data *= np.random.normal(loc=1, scale=noise, shape=y_data.shape)  # doesn't actually work, but you get the idea
    ... # more things here
    assert result == expected_values

antonykamp · 2020-06-15T20:04:40Z

Alright!
The next commit will contain the mentioned collection conf.py of variables and parameters + the integration into the individual test files.

tests/test_finite_difference.py

* Added HadamardPower printing * Import HadamardPower only for sympy.__version__ > (1,4) * Fixed the HadamardPower printer. Co-authored-by: Martin Roelfs <u0114255@kuleuven.be>

Adjust auto_variables import

antonykamp · 2021-03-08T16:54:50Z

I removed TC and COBYLA from no_constraints for the moment. I will open an issue about wrapping the remaining scipy methods in the next days.

About removing TC + CNM: Pytest implemented skipif to exclude a parametrized testcases under defined circumstances. But we can't parametrize one variable (eg constraints) and exclude the combination TC + CMN, because our "circumstances" refer to another parametrize variable (eg minimizer).
But we could parametrize a tuple of a minimizer and constraint and exclude the tuple TC, CMN. In this case, I would separate the definition of the constraints and the parametrization like this:

small_constraint = [Le(a,b)]
big_constraint = [
        Le(a, b),
        CallableNumericalModel.as_constraint(
            {z: ge_constraint}, connectivity_mapping={z: {a}},
            constraint_type=Ge, model=Model({y: a * x ** b})
        )
    ]

@pytest.mark.parametrize('minimizer, constraints',
    # Added None to check the defaul objective
    # Removed Truple with TrustConstr, because it cannot handle CallableNumericalModel
    [(min, constr) for (min, constr) in itertools.product(list(subclasses(ConstrainedMinimizer) | {None}), [small_constraint, big_constraint]) \
        if (min,constr) != (TrustConstr, big_constraint)]
    )
...

Another idea is a separate test for TrustConstr with constraints :D

Concerning LL: ge_constraints equals the function in the old unittests. 🙆‍♂️

pckroon · 2021-03-09T08:47:13Z

Heyhey!

Another idea is a separate test for TrustConstr with constraints :D

I don't really like the code snippet you suggest, since it puts a lot of logic in the "wrong" place. Separating TC + constraints is almost the correct idea I think: it's not TC+constraints, or even TC+CNM that's the exception, but CallableNumericalModel constraints are the exception. In summary, test 1: all constrained minimizers with symbolic constraints; test 2: all constrained minimizers except TC with CNM constraints.

You may want to create a separate helper function that performs the actual test.

Insert MINPACK test comments and alignments

antonykamp · 2021-03-09T10:55:00Z

I would prefer fixing it in FitResult itself (https://github.com/DuckNrOne/symfit/blob/master/symfit/core/fit_results.py#L42)

Should I open another issue because of LBFGSB (next to the issue with TrustConst and COBYLA) ?

pckroon

Very nice! A few small comments and then I think I'm actually happy. Before then I'll also go over all the changes again to refresh my memory though.

Could you move _run_tests to the top of the file?
For all tests you have expected_got, did you mean expected_gof (goodness of fit)?
Do we also want to test with objective=None? Or is that more for testing the Fit object, i.e. another test file/PR.

tests/test_fit_result.py

tests/test_constrained.py

pckroon · 2021-03-09T11:16:29Z

I would prefer fixing it in FitResult itself (https://github.com/DuckNrOne/symfit/blob/master/symfit/core/fit_results.py#L42)

Should I open another issue because of LBFGSB (next to the issue with TrustConst and COBYLA) ?

Sounds good to me.

antonykamp · 2021-03-09T12:00:00Z

For all tests you have expected_got, did you mean expected_gof (goodness of fit)?

It's propably a typo :) sorry for that! :D

Do we also want to test with objective=None? Or is that more for testing the Fit object, i.e. another test file/PR

I think I've added None, because I wanted to select the "default" objective.

pckroon

Almost there! There's still a few things to polish off.
Also, I don't see any place where you have None as objective. You do have it as minimizer though. Could you add a case or 2?

Once this is done this is suddenly a very thorough test set, I'm very happy with it!

tests/test_fit_result.py

pckroon · 2021-03-10T09:53:06Z

tests/test_fit_result.py

        # Test objective LogLikelihood
-        (dict(objective=LogLikelihood, x=np.linspace(1, 10, 10)), {a: 62.56756, b: 758.08369}, # TODO Adjust x_data
-         {a: float('nan'), b: float('nan')}, LogLikelihood, 
+        (dict(objective=LogLikelihood, x=np.linspace(1, 10, 10)),  # TODO Adjust x_data


Still TODO?

Kind of. This PR shouldn't be merged before we have good data for LL so that every test passes.

…erged constraints tests

antonykamp · 2021-03-17T19:34:23Z

Experiment and pick your favourite I'd say.

I decided to add an if-clause to the test-methods because we did the same thing with TrustConstr.
Now the only thing left to fix are the LogLikelihood-test-cases and the status_message of LBFGSB-test-cases (#325).

pckroon

I just went over the complete PR (again), and it's looking great!
Besides the new comments, here's what I think still needs doing:

Rebase on master
Find working LL parameters/cases
Rethink the constrained test cases critically so we can be sure the constraints are doing something.

Very short list :D

pckroon · 2021-03-29T08:39:33Z

requirements.txt

-scipy >= 1.0
+scipy >= 1.6


I don't think this should be in this PR.

pckroon · 2021-03-29T08:40:30Z

tests/auto_variables.py

+w, x, y, z = sf.variables('w, x, y, z')
+a, b, c, d = sf.parameters('a, b, c, d')


As alternative, call initVariables and initParameters. And I must say I'm not super thrilled with the camelCase naming of those two.

Pytest doesn't like calling fixtures directly :/

Fixture "init_variables" called directly. Fixtures are not meant to be called directly,
but are created automatically when test functions request them as parameters.

Ah ok. Then leave it as is.

pckroon · 2021-03-29T08:41:13Z

tests/test_fit_result.py

 import pytest
 import pickle
 from collections import OrderedDict

+from sympy.core.relational import Eq
+
+from tests.auto_variables import a, b, x, y, z


For the autofixtures to work, don't you need to import init_parameters and init_variables as well?

You are right 🤔 It's crazy that it worked without it

pckroon · 2021-03-29T08:43:02Z

tests/test_fit_result.py

+
+    dumped = pickle.dumps(fit_result)
+    new_result = pickle.loads(dumped)
+    assert sorted(fit_result.__dict__.keys()) == sorted(new_result.__dict__.keys())


Nit

Suggested change

assert sorted(fit_result.__dict__.keys()) == sorted(new_result.__dict__.keys())

assert set(fit_result.__dict__.keys()) == set(new_result.__dict__.keys())

pckroon · 2021-03-29T08:45:26Z

tests/test_fit_result.py

+
+
+@pytest.mark.parametrize('minimizer',
+        # Removed TrustConstr and COBYLA because their results differ greatly #TODO add them if fixed


Is this still true?

Not anymore 🙌 The results of COBYLA and TrustConstr are correct with more iterations.

Edit: TrustConstr runs with LogLikelihood, 1e8 as the maximum number of iterations and no constraints forever. I think that's because of the LL, but we should keep an eye on it.

pckroon · 2021-03-29T08:54:04Z

tests/test_fit_result.py

+        ([Le(a, b)],
+         dict(x=np.linspace(1, 10, 10), y=3 * np.linspace(1, 10, 10) ** 2),
+         {a: 2.152889, b: 2.152889}, {a: 0.2371545, b: 0.05076355}, 
+         LeastSquares,
+         {'r_squared': 0.99791, 'objective_value': 98.83587, 'chi_squared': 197.671746}),


I'm not completely sure this is the best possible model, since it really has only 1 minimum. Maybe a case with 2 degenerate global minima, where the constraint selects which one to find would be better? OTOH, this seems to work fine, so leave it.

pckroon · 2021-03-29T08:56:36Z

tests/test_fit_result.py

+        ([CNM_CONSTRAINT],
+         dict(x=np.linspace(1, 10, 10), y=3 * np.linspace(1, 10, 10) ** 2),
+         {a: 3, b: 1.9999999}, {a: 1.4795115e-8, b: 2.28628889e-8}, 
+         LeastSquares,
+         {'r_squared': 1.0, 'objective_value': 1.870722e-13, 'chi_squared': 3.741445e-13}),


Same comment here, how can we be sure the CNM_CONSTRAINT does anything, since the outcome is the same as without constraint.

pckroon · 2021-03-29T08:56:49Z

tests/test_fit_result.py

+         LogLikelihood,
+         {'objective_value': -float('inf'), 'log_likelihood': float('inf'), 'likelihood': float('inf')}),
+        # No special objective
+        ([Le(a, b), CNM_CONSTRAINT],


tests/test_fit_result.py

pckroon · 2021-03-29T08:58:51Z

tests/test_fit_result.py

+    _run_tests(fit_result, expected_par, expected_std, expected_obj, expected_gof)
+
+
+def test_fitting_2():


Do you also want to move this to the parametrized version, or rather keep it as is?

I'm not even sure about the benefit of this test :D

antonykamp · 2021-03-31T06:54:55Z

Thank you for the review (again) 🚀
Regarding the constraints, I would say that we only edit CNM_CONSTRAINT in that way, that the outcome isn't the same as without it. But edit in which way? 🤔

pckroon · 2021-03-31T11:59:53Z

Regarding the constraints, I would say that we only edit CNM_CONSTRAINT in that way, that the outcome isn't the same as without it. But edit in which way? 🤔

By using a different Model. For example, if you're not fitting but just minimizing a function, that function can have multiple (local) minima. The combination of initial guesses and constraints will decide which one you'll find. When fitting functions it's a bit harder to see, but I think we can achieve the desired effect by taking a model with degenerate/redundant parameters (but I'm not completely sure here). For example: {z: (a+b+c)*x. That would make all combinations where a+b+c = d have the same outcome. I think we can use this example to have constraints that cause, e.g. a < b < c. We may need to tweak the test a bit in case we need to change the initial guesses or bounds. If it gets too complicated, leave it out of this PR.

antonykamp · 2021-04-01T09:51:49Z

Thank you for the explanation! I'll look what I can do with this new information :)

antonykamp · 2021-04-02T14:30:13Z

Alright :D It doesn't matter what I try the results don't change :)) I'm just not in the topic :) sooorry :)

pckroon · 2021-04-06T08:07:28Z

No problem. Could you rebase your branch on master? I can then take a look at the LL testcases, and maybe the constrained ones as well.

antonykamp · 2021-04-12T05:54:39Z

Thank you very much for doing this! :)
Just as a note: I've excluded TrustConstr because the LL test case didn't stop with 1e8 iterations. :)

antonykamp · 2022-12-13T20:40:42Z

Closed because it's old :)

antonykamp and others added 3 commits December 10, 2019 16:13

Merge pull request #4 from tBuLi/master

0130aa5

Update Local Repo

Improve test_finite_difference

346dee6

Improve test_fit_result

8870043

pckroon requested changes Dec 27, 2019

View reviewed changes

tests/test_finite_difference.py Outdated Show resolved Hide resolved

tests/test_finite_difference.py Outdated Show resolved Hide resolved

tests/test_finite_difference.py Outdated Show resolved Hide resolved

antonykamp added 2 commits December 27, 2019 20:38

Requested Changes

be72272

Changed method name

6b867e5

Reduced number of fixtures

5db02b4

pckroon requested changes Jun 16, 2020

View reviewed changes

tests/test_finite_difference.py Outdated Show resolved Hide resolved

tests/test_finite_difference.py Outdated Show resolved Hide resolved

tBuLi and others added 6 commits June 19, 2020 21:50

Added HadamardPower printing (tBuLi#300)

3b8d832

* Added HadamardPower printing * Import HadamardPower only for sympy.__version__ > (1,4) * Fixed the HadamardPower printer. Co-authored-by: Martin Roelfs <u0114255@kuleuven.be>

test_multi_indep - correct spelling

de435b1

test_model - add sec parametrize

12d376c

test_finite_difference - remove prefix "_", relDiv

5081e78

Added global collection of variables/parameters

51b4d64

Added global collection to finite_difference

2741406

Adjust auto_variables import

antonykamp added 3 commits March 9, 2021 11:31

Moved method 'subclasses' to an external file

bd5a09c

Excludes constraint CMN test

29d589c

Insert MINPACK test comments and alignments

Import subclass from rec_subclass instead test_minimizer

4dd3768

pckroon requested changes Mar 9, 2021

View reviewed changes

antonykamp added 2 commits March 9, 2021 16:50

renamed rec_subclasses to helper_functions

eb46d97

typo,alignment,parametrize constraint,move _run_test

43041bb

pckroon requested changes Mar 10, 2021

View reviewed changes

check stdevs; added None as objective, more iterations with COBYLA; m…

4cccf00

…erged constraints tests

antonykamp mentioned this pull request Mar 22, 2021

Fit does not warn when constraints are given with a minimizer that does not support them #266

Open

update scipy

4dd2847

pckroon requested changes Mar 29, 2021

View reviewed changes

antonykamp added 2 commits March 29, 2021 21:57

Implement review I

1967b30

remove trustconstr

9fc1fc6

antonykamp added 3 commits April 12, 2021 07:25

add TrustConstr partly to test_no_constraint

14af578

rebase on master

92ed15c

Added 1e8 iterations to TrustConstr

5c417b9

antonykamp closed this Dec 13, 2022

		w, x, y, z = sf.variables('w, x, y, z')
		a, b, c, d = sf.parameters('a, b, c, d')

	assert sorted(fit_result.__dict__.keys()) == sorted(new_result.__dict__.keys())
	assert set(fit_result.__dict__.keys()) == set(new_result.__dict__.keys())



		@pytest.mark.parametrize('minimizer',
		# Removed TrustConstr and COBYLA because their results differ greatly #TODO add them if fixed

		_run_tests(fit_result, expected_par, expected_std, expected_obj, expected_gof)


		def test_fitting_2():

Improve test_finite_difference and test_fit_result #289

Improve test_finite_difference and test_fit_result #289

Conversation

antonykamp commented Dec 25, 2019

antonykamp commented Dec 25, 2019

pckroon left a comment

Choose a reason for hiding this comment

antonykamp commented Dec 27, 2019

antonykamp commented Dec 27, 2019

pckroon commented Dec 28, 2019

antonykamp commented Dec 29, 2019 • edited Loading

pckroon commented Dec 29, 2019

antonykamp commented Dec 29, 2019 • edited Loading

pckroon commented Dec 30, 2019

antonykamp commented Dec 30, 2019 • edited Loading

pckroon commented Dec 31, 2019

antonykamp commented Dec 31, 2019 • edited Loading

antonykamp commented Jun 14, 2020

pckroon commented Jun 15, 2020

antonykamp commented Jun 15, 2020 • edited Loading

pckroon commented Jun 15, 2020

antonykamp commented Jun 15, 2020

antonykamp commented Mar 8, 2021

pckroon commented Mar 9, 2021

antonykamp commented Mar 9, 2021 • edited Loading

pckroon left a comment

Choose a reason for hiding this comment

pckroon commented Mar 9, 2021

antonykamp commented Mar 9, 2021

pckroon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonykamp commented Mar 17, 2021

pckroon 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

antonykamp commented Mar 31, 2021

pckroon commented Mar 31, 2021

antonykamp commented Apr 1, 2021

antonykamp commented Apr 2, 2021

pckroon commented Apr 6, 2021

antonykamp commented Apr 12, 2021

antonykamp commented Dec 13, 2022

antonykamp commented Dec 29, 2019 •

edited

Loading

antonykamp commented Dec 29, 2019 •

edited

Loading

antonykamp commented Dec 30, 2019 •

edited

Loading

antonykamp commented Dec 31, 2019 •

edited

Loading

antonykamp commented Jun 15, 2020 •

edited

Loading

antonykamp commented Mar 9, 2021 •

edited

Loading