-
Notifications
You must be signed in to change notification settings - Fork 330
Feature: matplotlib renderer #1700
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feature: matplotlib renderer #1700
Conversation
- initial implementation based on @LandyQuack's initial code - some simple test cases
- implement "dejavu sans" -font so "-" is printed without error
- simple speed test
|
You should add matplotlib as optional dependency on the |
You can execute the test using |
|
You will have to remove the import from init.py |
Tried to do that with latest commit. However, the assertion fails since the svg generated plot is different in pdf as the direct rendered, even with the ignore flags. I think it's enough to just have comparison pictures with the two methods, so that possible differences can be spotted |
|
Hi @petri-lipponen-movesense 🙂 👋 Thank you for working on this nice contribution that could be useful to many
I tested your branch locally, and enabled the It works totally fine! 🙂 There is the relevant code snippet: from test.conftest import assert_pdf_equal
HERE = Path(__file__).resolve().parent
FONT_FILE = HERE / "../fonts/DejaVuSans.ttf"
DEFAULT_MPL_BACKEND = plt.get_backend()
def create_fpdf(w_mm, h_mm):
pdf = FPDF(unit="mm", format=(w_mm, h_mm))
pdf.add_font("dejavu sans", "", str(FONT_FILE))
pdf.add_page()
font_manager.fontManager.addfont(str(FONT_FILE))
mpl.rcParams["font.sans-serif"] = ["dejavu sans"]
return pdf
def test_mpl_figure_with_simple_backend(tmp_path):
plt.switch_backend(DEFAULT_MPL_BACKEND)
w_inch = 4
h_inch = 3
w_mm = w_inch * 25.4
h_mm = h_inch * 25.4
fig = gen_fig(w_inch, h_inch)
svg_buffer = io.BytesIO()
fig.savefig(svg_buffer, format="svg")
svg_buffer.seek(0)
pdf_svg = create_fpdf(w_mm, h_mm)
pdf_svg.image(svg_buffer, x=0, y=0, w=w_mm, h=h_mm)
assert_pdf_equal(
pdf_svg,
HERE / "mpl_figure_with_simple_backend.pdf",
tmp_path
)
def test_mpl_figure_with_fpdfrenderer(tmp_path):
plt.switch_backend("module://fpdf.fpdf_renderer")
w_inch = 4
h_inch = 3
w_mm = w_inch * 25.4
h_mm = h_inch * 25.4
fig = gen_fig(w_inch, h_inch)
pdf_fpdf = create_fpdf(w_mm, h_mm)
scale = float(w_mm / fig.bbox.width)
origin = (0, 0 + h_mm) # FPDF uses bottom-left as origin
fig.savefig(fname=None, fpdf=pdf_fpdf, origin=origin, scale=scale)
assert_pdf_equal(pdf_fpdf, HERE / "mpl_figure_with_fpdfrenderer.pdf", tmp_path) |
| ## [2.8.6] - Not released yet | ||
| ### Added | ||
| * support for SVG `<linearGradient>` and `<radialGradient>` elements - _cf._ [issue #1580](https://github.com/py-pdf/fpdf2/issues/1580) - thanks to @Ani07-05 | ||
| * Direct FPDF renderer for matplotlib images |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a link there to the new documentation section in https://py-pdf.github.io/fpdf2/Maths.html#using-matplotlib ?
| *.*~ | ||
| *.swo | ||
| *.swp | ||
| test/mpl_renderer/generated_pdf/*.pdf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to add this
| @@ -0,0 +1,437 @@ | |||
| """ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please rename this file matplotlib_renderer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like we should have an adaptors or integrations folder, and create matplotlib.py inside.
Then when we implement other adaptors like the the pandas one on #1046 it goes on the same subfolder
fpdf/fpdf_renderer.py
Outdated
| PT_TO_MM = 0.3527777778 # 1 point = 0.3527777778 mm | ||
|
|
||
|
|
||
| _log = logging.getLogger(__name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| _log = logging.getLogger(__name__) | |
| LOGGER = logging.getLogger(__name__) |
In fpdf2 source code we prefer to use UPPERCASE variable names for constants, without any underscore prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needed rename all over, committed
|
|
||
|
|
||
| class RendererTemplate(RendererBase): | ||
| """Removed draw_markers, draw_path_collection and draw_quad_mesh - all optional, we can add later""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a dosctring describing this class role & behaviour, more of a development comment.
| """Removed draw_markers, draw_path_collection and draw_quad_mesh - all optional, we can add later""" | ||
|
|
||
| def __init__(self, figure, dpi, fpdf, scale, transform, fig_width, fig_height): | ||
| del fig_height # unused for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| del fig_height # unused for now | |
| if fig_height: | |
| LOGGER.warn("fpdf2 matplotlib renderer currently ignores fig_height") |
I think that printing a warning could help a lot of end users,
that could not understand why a fig_height is ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it but it causes a lot of log spam. Ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those logs come from the call to .savefig()?
You can wrap this call in a with caplog.at_level(logging.INFO): context-manager then.
You'll find example of this approache in test/image/image_types/test_insert_images.py and test/image/png_images/test_png_file.py for example.
| ] | ||
|
|
||
| [project.optional-dependencies] | ||
| matplotlib = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please move this to the test group, as previously suggested by @andersonhc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already there, isn't it? The separate matplotlib group is handy if you want to install it in requirements.txt as "fpdf2[matplotlib]"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, but this should be documented somewhere 🙂
| default_backend = plt.get_backend() | ||
| HERE = Path(__file__).resolve().parent | ||
| GENERATED_PDF_DIR = HERE / "generated_pdf" | ||
| os.makedirs(GENERATED_PDF_DIR, exist_ok=True) | ||
| font_file = HERE / "../fonts/DejaVuSans.ttf" | ||
| logging.getLogger("fpdf.svg").setLevel(logging.ERROR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| default_backend = plt.get_backend() | |
| HERE = Path(__file__).resolve().parent | |
| GENERATED_PDF_DIR = HERE / "generated_pdf" | |
| os.makedirs(GENERATED_PDF_DIR, exist_ok=True) | |
| font_file = HERE / "../fonts/DejaVuSans.ttf" | |
| logging.getLogger("fpdf.svg").setLevel(logging.ERROR) | |
| DEFAULT_MPL_BACKEND = plt.get_backend() | |
| HERE = Path(__file__).resolve().parent | |
| FONT_FILE = HERE / "../fonts/DejaVuSans.ttf" |
- please use UPPERCASE variables for module constants
- please use simple
HEREand notHERE / "generated_pdf"to store reference PDF files. - please do not change the logging level from inside a une test file.
I saw that very verbose debug log lines were produced when running the unit tests:
WARNING matplotlib.font_manager:font_manager.py:1425 findfont: Generic family 'sans-serif' not found because none of the following families were found: Arial
I think we should figure a solution to this instead of hiding those lines, as they will bother other end users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The GENERATED_PDF_DIR doesn't have refernece pdf's, all pdfs are generated as a part of the test case. As written above, it's not really possible to compare svg vs renderer pdfs automatically (just too different).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other stuff fixed now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in my other comment #1700 (comment), please use the assert_pdf_equal() function as documented in the docs: https://py-pdf.github.io/fpdf2/Development.html#assert_pdf_equal-writing-new-tests
No need to generate extra PDFs in this GENERATED_PDF_DIR 🙂
|
|
||
| line_width = gc.get_linewidth() | ||
| line_width_px = line_width * self.dpi / 72.0 # points to pixels | ||
| mm_line_width = line_width_px * self._scale |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you checked that this works even if the unit passed to FPDF() contructor is not mm?
Could we please add a unit test that uses this new renderer with a FPDF() instance created with unit="pt"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably doesn't work ATM. Such a unit test and related fixes would definately be needed before this is not experimental. Unfortunately, I only have today to do the last fixes before holidays, so might take a while...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no hurry, this can be merged after the holidays 🙂
We don't usually release experimental feature in fpdf2.
We could consider releasing this Matplotlip renderer without the support for other units, but then there should be at least an error raised and a unit test covering this case.
Seems to be leftover of the original code. committed. Co-authored-by: Lucas Cimon <[email protected]>
Co-authored-by: Lucas Cimon <[email protected]>
I'm sure the testing that svg is identical to svg and renderer is identical to renderer works (and may be useful for preventing accidental changes in appearance). My point was to try to compare svg plot to renderer plot, which didn't work... |
OK I understand what you were trying to achieve. It's OK if that's not possible, but yes having some unit tests covering this new code andcalling
We have some documentation regarding |
Implemented a fpdf_renderer for directly drawing matplotlib images to fpdf document. This work is based on the initial implementation of @LandyQuack in issue #789.
Checklist:
A unit test is covering the code added / modified by this PR
In case of a new feature, docstrings have been added, with also some documentation in the
docs/folderA mention of the change is present in
CHANGELOG.md[EXPERIMENTAL] This PR is ready to be merged as EXPERIMENTAL
Some more open details:
By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.