search

flav-io / flavio

A Python package for flavour physics phenomenology in the Standard model and beyond
http://flav-io.github.io/
MIT License
71 stars 62 forks source link

enable contour plots with true minimum #113

Closed peterstangl closed 4 years ago

peterstangl commented 4 years ago

This PR allows using an exact minimum determined by numerical minimization in contour plots generated by the flavio.plots.contour function. To this end, the flavio.plots.likelihood_contour_data now returns its z-values without subtracting the lowest z-value on the grid.

Consequently, the output of flavio.plots.likelihood_contour_data is not backward compatible with older versions of flavio.plots.contour. However, I don't see a reason why one should generate data with the new version but plot this data with an old version. On the other hand, flavio.plots.contour is fully backward compatible in the sense that plot data generated by old versions of flavio.plots.likelihood_contour_data can still be plotted with the new version of flavio.plots.contour. Furthermore, both flavio.plots.contour and flavio.plots.likelihood_contour_data are backward compatible in the sense that code written for the old versions of these functions will still work with the new versions.

(The only exception is calling flavio.plots.contour and using the argument interpolation_factor not as a keyword argument. i.e. if the function is called as contour(x, y, z, levels, interpolation_factor) instead of contour(x, y, z, levels, interpolation_factor=interpolation_factor), then this yields unexpected results in the new version since the 5th argument of flavio.plots.contour now is z_min. This could be avoided if z_min is made the last argument and all other arguments are kept in their previous order. However, I thought that z_min might be a rather frequently used argument and function calls like contour(x, y, z, levels, z_min) could be convenient. But I am not completely sure about this. @DavidMStraub do you have an opinion on that?)

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.005%) to 93.878% when pulling ecfc66b1070c4cfd84ab488e630f8808b53fb32b on peterstangl:plotfunctions into b91383c1db3fcbcd1a55653f56d2ce37cae92be5 on flav-io:master.

peterstangl commented 4 years ago

There was a bug in the initial commits that caused the flavio.plot.contour function to set z_min = np.min(z) if the z_min argument was set to 0. This has been fixed in 12ca043.

DavidMStraub commented 4 years ago

@DavidMStraub do you have an opinion on that?

In my opinion it's always safer (as a user) to use keyword arguments when more than 3 or 4 arguments are involved. In fact, this could be enforeced, and any backwards-incompatible ambiguities avoided, by defining

def contour(x, y, z, levels, *, z_min, ...

although this would prevent calls contour(x, y, z, levels, z_min) without keywords (but I'm not sure I see the benefit).

peterstangl commented 4 years ago

@DavidMStraub do you have an opinion on that?

In my opinion it's always safer (as a user) to use keyword arguments when more than 3 or 4 arguments are involved. In fact, this could be enforeced, and any backwards-incompatible ambiguities avoided, by defining

def contour(x, y, z, levels, *, z_min, ...

although this would prevent calls contour(x, y, z, levels, z_min) without keywords (but I'm not sure I see the benefit).

Thanks for the suggestion! I actually didn't know that one can enforce the use of keyword arguments. I think this is really a good solution to avoid backward-incompatible ambiguities. It's now implemented in ecfc66b.