| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
Is this new behaviour? Or did it get broken in the colorbar overhaul?
Sorry, something went wrong.
No idea. Seems to make sense to have it the "new" way, but maybe I miss something. Should be pretty straightforward to add a keyword argument to enable/disable this if required. Edit: seems like new behavior, at least compared to the last time someone touched those lines. #20054 Edit 2: seems like the concept of removing the first and last element was introduced in 4d1107c (see the _edges method, later simplified and inlined). @efiring close to 16 years ago, but do you remember any design decision here? So probably we would like to introduce a keyword argument for this? Still, I tend to think that the separators are expected when extending, so maybe it should be on by default? I also assume that a release note should be included then? |
Sorry, something went wrong.
|
Thinking a bit more about it, it seems like one would like to have the option to disable this. For example if the extend part has the same color as the adjacent block. A good name for the keyword argument? |
Sorry, something went wrong.
|
IMHO it could be sufficient to draw a line if the extent color (i.e. over/under) is different and not draw a line if the color is the same. That should almost always be the desired behavior. We should not unnecessarily bloat our API with options that nobody needs. We can always add a kwarg later if needed. |
Sorry, something went wrong.
I've also come to that conclusion after looking at more examples. |
Sorry, something went wrong.
|
Further, I'm not sure this is needed at all. I think this is new behaviour, and it may break as many people as it helps. If the extend color is the same, it is probably unwanted to have a break; if the extend color is different, there is a clear visual break anyways. So I'm not sure that there is a lot of semantic aid this is giving that's worth changing behaviour? I'm also against all the special flags and duck typing and checks we have for colorbar - its API has really gotten out of hand over the years. |
Sorry, something went wrong.
|
@jklymak I'm unclear whether you agree with my proposal (not configurable but line/no line depends on the colors) or whether you argue for always no line. I agree neither is a question of semantics, but I think my proposal is better from an aesthetic perspective. |
Sorry, something went wrong.
|
I wouldn't add the line... |
Sorry, something went wrong.
|
I never use drawedges, and I don't know what the old behavior was (but see below). But logically, I think that if drawedges is requested, then there should be a line at the base of the triangle, regardless of whether the color changes. It's a boundary with a logical meaning, no different from any of the other boundaries, which also might or might not coincide with a color change. The user has said "draw lines at boundaries", so just do it. I strongly oppose putting in color-checking logic or a kwarg. I'm not strongly opposed to rejecting the change entirely and saying "we will leave this minor inconsistency in place", but I think #22864 is a valid issue. History: quickly looking at 4d1107c, I think this is consistent with drawing edges at the triangle bases. The _edges method is omitting the ends of the bar, which would be the triangle tips when there are extensions, or the actual colorbar ends when there are no extensions. The entire colorbar was drawn as a single pcolormesh; the extensions were not tacked on separately. In other words, I think that the current behavior is new, in which case the current PR is fixing a regression. |
Sorry, something went wrong.
|
OK agreed, this was a regression - if you checkout 3.4.2 then you get the extra lines. So a simple fix as proposed here should be correct. |
Sorry, something went wrong.
|
Thanks for the feedback! Will update the PR to check if the extend parts have different or the same color as well. |
Sorry, something went wrong.
|
This is ready to merge when you make the change above, right? |
Sorry, something went wrong.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
|
Since I messed up and pushed this to the base repo rather than my own and I then changed things to avoid that, I am not really sure how to edit this properly. Hence, I would like to ask for a squash merge. (Also, the alpha test promised in #23260 (comment) is ideally postponed.) |
Sorry, something went wrong.
|
I think the doc build failure is because I pushing this to the main repo. Especially since I didn't change anything related to the docs... |
Sorry, something went wrong.
|
This was a regression from 3.4.2 according to @jklymak, so it should be backported to v3.5.x. @meeseeksdev backport to v3.5.x |
Sorry, something went wrong.
…865-on-v3.5.x Backport PR #22865 on branch v3.5.x (Fix issue with colorbar extend and drawedges)
| Back | FazBrowse Home | New Git URL |
PR Summary
Closes #22864
Maybe the test can be simplified/written in a better way?
PR Checklist
Tests and Styling
Documentation