Repository navigation
Fix WCSAxes bbox calculation - #10797
Conversation
|
This was deceptively easy in the end, just needed to include the bbox of the actual axes instead of just the tick labels... |
|
Needs a change log. I'll let reviewers decide if a test is necessary. Thanks! |
|
Thank for the fix! It looks like no tests cover this line - though I suspect they do, but it might just be that we don't report coverage from CircleCI? Could you add an explicit test of the bounding box, which isn't an image test? |
|
The tests seem to be failing, otherwise looks good 👍 |
astrofrog
left a comment
astrofrog
left a comment
There was a problem hiding this comment.
Thanks! Just a small comment below.
|
@dstansby - do you have time to wrap this up? It would be good to get this fix in! |
|
Not really, if someone wants to finish it feel free. I think the outstanding issue is that the bbox calaculation is matplotlib version dependent, so the test should be pinned to and only run on a single version of matplotlib. |
| mpl_version = Version(matplotlib.__version__) | ||
| MATPLOTLIB_LT_21 = mpl_version < Version("2.1") | ||
| MATPLOTLIB_LT_22 = mpl_version < Version("2.2") | ||
| NOT_MATPLOTLIB_33 = mpl_version.major != 3 or mpl_version.minor != 3 |
There was a problem hiding this comment.
Can you make this MATPLOTLIB_33 and write not MATPLOTLIB_33 in the skipif below?
astrofrog
added
the
zzz 💤 merge-when-ci-passes
Fix WCSAxes bbox calculation
4 participants
Fixes #10796.
This includes the axes bounding box in the caclulation; previously only the tick label bboxes were included.