MNT: privatize colorbar attr #21933
MNT: privatize colorbar attr #21933
Conversation
The *filled* kwarg doesn't directly do anything and filling is completely controlled by the mappable. OTOH, maybe we want to reinstate the ability of *filled* to fill contours?
|
@anntzer you did some to the previous work here. |
I guess I'm not really sure what a filled colorbar for a nonfilled contourset would look like, as one specifies colors for intervals and the other specifies colors for edges? I'm OK with the change here (modulo deprecation note, yada yada), although it's unclear whether we'll want to restore filled as a separate parameter later (I'll defer to your judgment on that), in which case that would probably partially undo this PR? |
|
Yes we should definitely decide if we want to reinstate the filling behaviour before releasing 3.6. I'm mildly against, otoh line-only colorbars are a bit hard to read so I can understand why folks want them filled. |
|
I agree that filled should be private. Modulo deprecation ... RE reinstantiate: Is it the task of the colorbar to decide that or should the ScalarMappable define how it would like its colorbar to be rendered? |
|
Maybe @greglucas has some thoughts? |
There was a problem hiding this comment.
No issues with this deprecation from me. Just a few minor comments that are tangential to this PR and could be addressed in a follow-up, like considering what we actually want public/private API of the Colorbar to look like. I'm just putting these comments down here so I don't forget about them later, but I don't think they are necessary for this PR.
- Do we need
update_ticks()at all? It says it shouldn't be used by the public, and It is 4 lines that could be put into the_draw_allmethod since that is the only place it is ever called. - Should
update_normal()be a private_update_normal()instead? I think this was needed by users when the norm information wasn't automatically updated/propagated to the Colorbar, but now this is taken care of by being attached to the norm's callback So, it is really an internal helper instead of public API.
| if isinstance(self.mappable, contour.ContourSet): | ||
| CS = self.mappable | ||
| if not CS.filled: | ||
| self.add_lines(CS) | ||
| self.stale = True | ||
|
|
||
| @_api.deprecated("3.6", alternative="fig.draw_without_rendering()") |
There was a problem hiding this comment.
Does draw_without_rendering() actually hit this path? I guess so when the axes this colorbar is attached to gets called? But, it isn't obvious to me.
| """ | ||
| self._draw_all() | ||
|
|
||
| def _draw_all(self): |
There was a problem hiding this comment.
Can we rename this to _update_parameters() or something like that instead? A quick search shows that the only other draw_all() is in pyplot to update all stale figures, which is very different from this draw_all.
As an aside, I'm wondering if this information should actually be in a draw(self, renderer) method instead (it looks like Colorbar doesn't have a draw() currently), that can then call self.ax.draw() after these updates.
|
This has two approvals, so I'll self merge.... |
PR Summary
In partial response to #21932: we deprecated
filledfrom clashing with the ContourSet mappable, but none of the other mappables can change filled, so we have basically made the argument useless. This PR simply deprecates the useless kwarg.The filled kwarg doesn't directly do anything and filling is completely controlled by the mappable.
OTOH, maybe we want to reinstate the ability of filled to fill ContourSets that are not filled - that is what #21932 would like?
Also deprecates
draw_allas a public method. This doesn't draw anything, and its is not used as public API:Other potential attributes to make read-only:
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).