Bugfix/plot crop #2592
Bugfix/plot crop #2592
Conversation
timtmok
force-pushed
the
bugfix/plot-crop
branch
from
94c34e9 to
f88f4b2
Compare
Render without bbox=tight option that format_display_data() uses
timtmok
force-pushed
the
bugfix/plot-crop
branch
from
f88f4b2 to
96d4643
Compare
timtmok
force-pushed
the
bugfix/plot-crop
branch
from
96d4643 to
20403ba
Compare
| @@ -197,14 +199,19 @@ def _resize_pickled_figure( | |||
|
|
|||
| figure.set_size_inches(width_in, height_in) | |||
|
|
|||
| format_dict, md_dict = format_display_data(figure, include=formats, exclude=[]) # type: ignore | |||
| # Render the figure to a buffer | |||
There was a problem hiding this comment.
Do we still need the figure.set_dpi(dpi)above given that we pass it to savefig too?
| @@ -197,14 +199,19 @@ def _resize_pickled_figure( | |||
|
|
|||
| figure.set_size_inches(width_in, height_in) | |||
|
|
|||
| format_dict, md_dict = format_display_data(figure, include=formats, exclude=[]) # type: ignore | |||
There was a problem hiding this comment.
Can we remove the format_display_data import too?
|
|
||
| plt.close(figure) | ||
|
|
||
| if was_interactive: | ||
| plt.ion() | ||
|
|
||
| return (format_dict, md_dict) | ||
| return format_dict |
There was a problem hiding this comment.
This error is because this function's return annotation is still Tuple[dict, dict], but it should be Dict[str, str], I think.
| format_dict, md_dict = self._resize_pickled_figure( | ||
| pickled, width_px, height_px, pixel_ratio | ||
| ) | ||
| format_dict = self._resize_pickled_figure(pickled, width_px, height_px, pixel_ratio) | ||
| data = format_dict["image/png"] |
There was a problem hiding this comment.
I believe this error is also due to the incorrect return type annotation on _resize_pickled_figure.
| data = format_dict["image/png"] | ||
| output = PlotResult(data=data, mime_type="image/png").dict() | ||
| figure_comm.send_result(data=output, metadata=md_dict) | ||
| figure_comm.send_result(data=output, metadata={"mime_type": "image/png"}) |
There was a problem hiding this comment.
Do you know if this mime_type key is used anywhere?
There was a problem hiding this comment.
I thought this is needed by the Plot Client to construct a suitable data uri for the image?
There was a problem hiding this comment.
I don't think this is currently used and I was thinking to remove it entirely from the comm definition. It seems that R might use this though? I had changes in Amalthea when I tried regenerating using generate-comms.ts.
There was a problem hiding this comment.
Or did @seeM mean the metadata property here?
There was a problem hiding this comment.
Yes I was referring to the metadata={"mime_type": "image/png"} .
|
|
||
| # Serialize the reference figure as a base64-encoded image | ||
| data_ref, _ = format_display_data(fig_ref, include=["image/png"], exclude=[]) # type: ignore | ||
| # data_ref, _ = format_display_data(fig_ref, include=["image/png"], exclude=[]) # type: ignore |
There was a problem hiding this comment.
| # data_ref, _ = format_display_data(fig_ref, include=["image/png"], exclude=[]) # type: ignore |
|
Thanks for the additional fixes @timtmok I tried out the branch and your examples in Fill mode and it looks good to me! |
Intent
Address #502
Original PR: github.com/posit-dev/positron-python/pull/432
Approach
The plot is requested with a size that will fill the plot pane. The resulting image was cropped and being displayed at 100% scale so it doesn't fill the pane. Calling format_display_figure was setting options that results in a cropped image.
Rendering the figure to a buffer and base64 encoding it is doing the same as format_display_figure but doesn't seem to crop the image. I don't know why it gets cropped or which option that causes it.
Another issue was setting BASE_DPI to 96 (100 is the matplotlib default) that caused a slight change in the resulting image size. Using 96dpi would convert a requested image size of 100x100 to 96x96@96dpi. I think this is a result of how matplotlib requires the image to be set in inches. We convert the resolution into inches but also changed the DPI. Since we are setting a lower DPI, there are less pixels per inch (aka DPI) and the image is smaller than requested.
QA Notes
This can be reproduced with a dynamic plot using code like this:
Switching the sizing to
Fillshould fill the Plot pane image area (minus space at the top of the image reserved for the progress bar; about 2 pixels).The current issue looks like this:
When rendered with the fix:
It's most noticeable in the
Fillmode but other modes also show how the image doesn't fill in the expected direction.