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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update the default internal value of clim to support Matplotlib #679

Merged
merged 4 commits into from Nov 30, 2021

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented Nov 19, 2021

PR in preparation of the upcoming support of Matplotlib and Plotly.

self._dim_ranges['c'] was set to either clim if not None or to (None, None). This caused no issue with the Bokeh backend of holoviews, which accepts a param.Tuple for clim and handles properly when it includes None. However both the Matplotlib and Plotly backends accept a param.NumericTuple only.

The Bokeh backend used to accept a param.NumericTuple only as well, but that was changed in holoviz/holoviews#4383. The right fix might be to add support to whatever the Bokeh backend supports to Matplotlib & Plotly with allowing them to accept a non-numeric tuple for clim, but since I don't know what exactly this PR was about I went for the simpler, and hopefully correct, solution.

An important question here would be, does the bokeh backend behave the same way when clim is set to (None, None) compared to (np.nan, np.nan)?

@jlstevens pinging you since we talked about it briefly already 馃檭

@coveralls
Copy link

coveralls commented Nov 22, 2021

Coverage Status

Coverage decreased (-1.6%) to 70.879% when pulling 1d236f4 on fix_clim_matplotlib into 17ce0cc on master.

@maximlt
Copy link
Member Author

maximlt commented Nov 22, 2021

Ok so my first attempt to solve this made the test suite fail :/ In the second and successful attempt, at least in light of the test suite, the clim option is set in the contours plot only if c was already computed/set. I have to try that now with the matplotlib backend.

@maximlt
Copy link
Member Author

maximlt commented Nov 22, 2021

It's working well with the matplotlib backend!

@maximlt
Copy link
Member Author

maximlt commented Nov 24, 2021

I think I've finally found an acceptable solution but let me know what you think about it.

It happens that what was done in #587, i.e. setting the clim opts to plots created by .contour (and .contourf since it uses .contour), was only relevant for .contourf. .contour has a first step that consists in calling .quadmesh which applies a redim to the object it returns, this effectively takes into account the clim parameter a user can pass in the hvplot API. However, it happens that the contours holoviews operation overrides the dimension range in the case of a filled contour:

https://github.com/holoviz/holoviews/blob/4ea310adcfb8fe061faf5acc732522e366dc01b1/holoviews/operation/element.py#L591-L593

So I've removed the line added by #587, and replaced it by a .redim.range only applied to the element returned by .contourf. Using .redim.range allows to fix the issue solved originally by this PR, i.e. to prepare the code for the Matplotlib backend which doesn't allow the clim tuple to contain any None.

@philippjfr philippjfr merged commit e0767f2 into master Nov 30, 2021
@maximlt maximlt deleted the fix_clim_matplotlib branch December 16, 2021 11:48
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.

None yet

3 participants