Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

enable contour plots with true minimum #113

Merged
merged 5 commits into from
Jul 31, 2020

Conversation

peterstangl
Copy link
Collaborator

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

coveralls commented Jul 30, 2020

Coverage Status

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

@peterstangl
Copy link
Collaborator Author

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.

Comment on lines 690 to 691
levelsf = [0] + list(levels)
zero_contour = min(np.min(z),np.min(levels)*(1-1e-16))
levelsf = [zero_contour] + list(levels)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The first solution doesn't work if all z values on the grid are larger than 0.
The new version works and solves a long-standing issue, namely that an error was produced if any of the levels was actually smaller than the zero_contour value (which initially was np.min(z)).

@DavidMStraub
Copy link
Contributor

@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
Copy link
Collaborator Author

@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.

@peterstangl peterstangl merged commit 7d34431 into flav-io:master Jul 31, 2020
@peterstangl peterstangl deleted the plotfunctions branch July 31, 2020 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants