Skip to content

Use mpl::format for error messages instead of pybind11 - #32484

Open
iccir wants to merge 2 commits into
matplotlib:mainfrom
iccir:mplutils
Open

Use mpl::format for error messages instead of pybind11#32484
iccir wants to merge 2 commits into
matplotlib:mainfrom
iccir:mplutils

Conversation

@iccir

@iccir iccir commented Oct 16, 2026

Copy link
Copy Markdown
Contributor

PR summary

From yesterday's meeting, it sounds like nanobind is a direction that we want to take. I think it might be easier to create smaller PRs for "preparing the repository for nanobind" rather than one huge PR. I can still do the huge PR if people prefer.

This PR adds mpl::format() to mplutils.h and uses it to format error messages rather than directly using pybind11::str.

Why?

nanobind's str class is simpler than pybind11's. There is no _s UDL nor implicit conversion to std::string. Hence, when porting to nanobind, the following code:

throw std::runtime_error(
    "Inverse transformed mesh array should be 2D not {}D"_s.format(
        output_mesh_array.ndim()));

becomes:

throw std::runtime_error(nb::str(
    "Inverse transformed mesh array should be 2D not {}D").format(
        output_mesh_array.ndim()).c_str());

With this PR, both the pybind11 and nanobind versions are the same:

throw std::runtime_error(mpl::format(
    "Inverse transformed mesh array should be 2D not {}D",
        output_mesh_array.ndim()));

This PR also makes the following changes:

  • mpl_round goes into the mpl namespace and is renamed to round. As far as I can tell, the mpl_ prefix was due to including this file from pure C in the 2010's.
  • mpl_round_to_int goes into the mpl namespace and is renamed to round_to_int.
  • Removed #ifdef __cplusplus // not for macosx.m. The old macOS backend doesn't include this file. The new macOS backend may include this file, but will do so from Objective-C++.
  • I moved the #includes higher in the file. I wish they were at the top, but I wasn't sure if they needed to be placed after the #define shenanigans.

Note: more code will be added to the mpl namespace in the future.

AI Disclosure

The robot taught me how to use std::forward several months ago.

PR quality check

  • Use an expressive title, e.g. "Fix title font property precedence"
  • New and changed code is tested
  • [/] Plotting related features are demonstrated in an example
  • [/] New features and API changes have release notes
  • [/] Documentation complies with general and docstring guidelines

@iccir
iccir marked this pull request as draft October 16, 2026 16:36
@iccir
iccir marked this pull request as ready for review October 16, 2026 16:52
QuLogic
QuLogic reviewed Oct 16, 2026
View reviewed changes

@QuLogic QuLogic left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we could depend on C++20, we could directly use std::format, but that seems to not be portable yet and SciPy still hasn't recommended it.

Changing the call sites to a similar signature does seem a good idea for that future, whenever it may be.

Comment thread src/_image_wrapper.cpp Outdated
Comment thread src/_image_wrapper.cpp Outdated
Comment thread src/_tkagg.cpp Outdated
Comment thread src/mplutils.h Outdated
@iccir
Fix indentation based on review
dc9df6a 
(plus a bonus tab-to-space fix)
@QuLogic QuLogic added the CI: Run cibuildwheel Run wheel building tests on a PR label Oct 16, 2026
@story645

story645 commented Oct 16, 2026

Copy link
Copy Markdown
Member

the coverage changes are real, so is there a way to mitigate that? even by using a no coverage pragma if it's not feasible to test?

@iccir

iccir commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

the coverage changes are real, so is there a way to mitigate that? even by using a no coverage pragma if it's not feasible to test?

I think I need some guidance or explanation here. I don't understand how the coverage has changed, other than there are less lines now and thus the already-not-handled branch is now a greater percentage?

story645
story645 approved these changes Oct 16, 2026
View reviewed changes

@story645 story645 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the namespacing, am concerned about the coverage (but also it could be a case of picking up stuff that wasn't ever covered)

@tacaswell

tacaswell commented Oct 16, 2026

Copy link
Copy Markdown
Member

Coverage data is based on HEAD dc9df6a (9 uploads) compared to BASE 003c1b5 (10 uploads)

We are short an upload to codecov and the ones I spot checked looked consistent with missing the appveyor (or some other windows) upload.

@story645

story645 commented Oct 16, 2026

Copy link
Copy Markdown
Member

and thus the already-not-handled branch is now a greater percentage?

commented before I saw your comment. Either that or it was left out of the initial coverage baseline and only got flagged in the coverage report b/c the code got changed. I think either this should get tests that those error messages get triggered or we can for now omit those files? Potentially a different discussion but we should maybe use a c++ coverage tool for the c++ so we can exclude specific c++ lines?

@tacaswell

tacaswell commented Oct 16, 2026

Copy link
Copy Markdown
Member

I am not worried about the decrease in qhull_wrapper as it is in a branch where we had to get qhull initialization to fail and it is definitely not worth the effort to force that to get coverage on error-message formatting!

@story645

story645 commented Oct 16, 2026

Copy link
Copy Markdown
Member

I am not worried about the decrease in qhull_wrapper as it is in a branch where we had to get qhull initialization to fail and it is definitely not worth the effort to force that to get coverage on error-message formatting!

My concern is coverage errors persisting and adding noise to the coverage reports going forward.

@iccir

iccir commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

We are short an upload to codecov and the ones I spot checked looked consistent with missing the appveyor (or some other windows) upload.

I feel like we are all starting to ignore AppVeyor because it frequently times out at the hour mark. Should we prioritize #32395 or try to fix it some other way?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell approved these changes
@QuLogic QuLogic QuLogic approved these changes
@story645 story645 story645 approved these changes

Assignees

No one assigned

Labels

backend: agg CI: Run cibuildwheel Run wheel building tests on a PR GUI: tk topic: contour topic: images topic: path handling topic: text/fonts

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@iccir @story645 @tacaswell @QuLogic