scikit-hep / probfit

Cost function builder. For fitting distributions.
http://probfit.readthedocs.io/
MIT License
51 stars 30 forks source link

Remove FakeFunc from AddPdf/AddPdfNorm._parts #91

Closed marinang closed 6 years ago

marinang commented 6 years ago

This is a workaround that worked for me and Following #84, I think it will solve a couple of tests.

marinang commented 6 years ago

So since there were few remaining test failures I tried to fix them as well.

marinang commented 6 years ago

So the builds work for python 2.7 and 3.4, and it works for me for python 3.7.

The only two tests that are remaining are two plot check that fail for python 3.5 and 3.6 and I cannot see the difference to solve (the diff plot is inside travis env).

eduardo-rodrigues commented 6 years ago

Hi @marinang, thumbs up for the heroic effort! I have not yet looked at the test_plottting.py file, which is where the failing tests come from, but one thing is really strange: at first all 81 test pass, ran with command python -m pytest -v. Then Travis seems to be rerunning all of test_plotting.py with the command python -m pytest -v --mpl tests/test_plotting.py, and there 2 tests fail. Anything obvious?

marinang commented 6 years ago

if you look at the makefile https://github.com/scikit-hep/probfit/blob/master/Makefile:

test: build python -m pytest -v python -m pytest -v --mpl tests/test_plotting.py

the second part is using pytest-mpl https://github.com/matplotlib/pytest-mpl, which is an image comparator. Basically it compares images produced in the tests functions and the images here https://github.com/scikit-hep/probfit/tree/master/tests/baseline.

The tests fail for /draw_residual_blh_norm.png and /draw_residual_ulh_norm.png which is highly mysterious why since it works for python 2.7 and 3.4. pytest-mpl produces a difference plot, which highlights where in the plots the differences are but we don't have access to them in the travis build right?

eduardo-rodrigues commented 6 years ago

Yep, got there as well, actually looking at the various files needed. While doing that I also noticed the comments

There is a slight difference in the x-axis tick label positioning for this plot between Python 2 and 3, it's not important here so increase the RMS slightly such that it's ignored

in a couple of tests in tests_plotting.py where a tolerance is specified. Could it be a matter of increasing the tolerance for the 2 failing tests?

marinang commented 6 years ago

@eduardo-rodrigues you have to put a big tolerance because the rms are ~45 and 56 for between the expected and obtained images. I put a big tolerance of 60 for those two test for py35 and py36, to make the test pass.

Two remaining problems still for py35 and py36, the doc is not compiled because of this module 'matplotlib.sphinxext.only_directives' which is not found. It should be found from the installed Matplotlib though ...

eduardo-rodrigues commented 6 years ago

Saw that. It's a weird thing the differences between Py 3.4 and 3.5-3.6. To be revisited at some point it seems …

I am tempted to accept this PR given that what is addressed is sorted-ish. The failure at creating the documentation is a different matter and deserves its own PR I would say. I browsed the web a bit and may try and address the problem with the doc soon.

I will merge this later unless somebody shouts.

marinang commented 6 years ago

@eduardo-rodrigues I just tried to set a maximum requirement on the matplotlib version in travis to the public one, because from the log I saw that the mil version was 3.0.1 ... so just a check

marinang commented 6 years ago

@eduardo-rodrigues for the doc according to https://matplotlib.org/3.0.0/devel/plot_directive.html?highlight=sphinx#module-matplotlib.sphinxext.plot_directive it looks like matplotlib.sphinxext.only_directive doesn't exist anymore.

eduardo-rodrigues commented 6 years ago

Yes, seems the directive is gone with matplotlib >=3.