fix colorbar region labels position according to region values #86
Conversation
- Use single method for colorbar creation. - Show only colorbar labels for regions that are actually visible in the subplot. - Set colorbar tick values and labels in conformity with self.values and visible regions and (vmin vmax) values.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
==========================================
+ Coverage 70.03% 73.66% +3.63%
==========================================
Files 5 5
Lines 327 338 +11
==========================================
+ Hits 229 249 +20
+ Misses 98 89 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Current colorbar labeling behaviour with 055825a
In my opinion, it could be good to take this opportunity to change the current naming of the parameter |
|
Thanks @zenWai - might not get to this until the end of the week/early next week unfortunately |
There was a problem hiding this comment.
Thanks a lot for this @zenWai
it could be good to take this opportunity to change the current naming of the parameter label_regions to something that represents the relation to the colorbar,
I agree, from my understanding of the codebase. Something like show_colorbar?
In general, I think this looks reasonable (I don't know the heatmap codebase very well! So feel free to push back and help me clarify my thinking). I've pointed out where I think the tests are hard to read IMO - maybe you could split them up a bit more into smaller tests, which would help me also understand your intentions better?
| settings.INTERACTIVE = False | ||
| settings.OFFSCREEN = True | ||
|
|
||
| # mock projected data for get_structures_slice_coords |
There was a problem hiding this comment.
Could the mock projected data be provided through a fixture?
| Checks that: | ||
| - colorbar display is controlled by show_cbar parameter. | ||
| - colorbar tick values and labels match visible regions. | ||
| - correct calls are made based on parameter combinations. |
There was a problem hiding this comment.
Would it be possible to split up into several tests?
The complicated parameters are quite hard to read and understand.
| ): | ||
| heatmap_2d.show(show_cbar=True) | ||
|
|
||
| if test_case.get("description") in [ |
There was a problem hiding this comment.
The long if ... elif ... elif ... else suggests to me that we should split this into several unit tests. I think this would also help with code legibility.
|
Hey @zenWai - let us know whether you're planning to respond/address my comments (no rush!) or you'd like the core team to take this on (when they find time) |
|
Thanks for the pings, I will have it ready by this weekend and I will be arround to finish it out |
Description
What is this PR
Why is this PR needed?
Issues with region labeling on colorbar #85
examples with PR change
heatmap_2d_subplots.pywithlabel_regions=True,How has this PR been tested?
/examples, other cases and the implemented test_heatmap_2d_colorbar.py
Is this a breaking change?
no
Does this PR require an update to the documentation?
no
Checklist: