Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Remove FakeFunc from AddPdf/AddPdfNorm._parts #91

Merged
merged 9 commits into from
Oct 31, 2018

Conversation

marinang
Copy link
Member

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

@marinang
Copy link
Member Author

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

@marinang
Copy link
Member Author

marinang commented Oct 30, 2018

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
Copy link
Member

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
Copy link
Member Author

marinang commented Oct 30, 2018

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
Copy link
Member

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
Copy link
Member Author

marinang commented Oct 31, 2018

@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
Copy link
Member

eduardo-rodrigues commented Oct 31, 2018

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
Copy link
Member Author

@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

from probfit.plotting import draw_pdf, draw_compare_hist
from probfit.pdf import gaussian, linear
from probfit.funcutil import rename
from probfit.functor import Extended, AddPdfNorm, AddPdf
from probfit.costfunc import UnbinnedLH, BinnedLH, BinnedChi2, Chi2Regression, \
SimultaneousFit

major, minor = sys.version_info[0:2]
if major >= 3 and minor >= 5:
special_tol = 60.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new tolerance seems excessively large. Why is that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@HDembinski HDembinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ok.

@marinang
Copy link
Member Author

@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
Copy link
Member

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants