FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Use mpl::format for error messages instead of pybind11 by iccir · Pull Request #32484 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

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

Merged
QuLogic merged 2 commits into
matplotlib:mainfrom
iccir:mplutils
Oct 20, 2026
ConversationCommits (2)ChecksFiles changed
Merged

Use mpl::format for error messages instead of pybind11#32484
QuLogic merged 2 commits into
matplotlib:mainfrom
iccir:mplutils

Conversation

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 marked this pull request as draft October 16, 2026 16:36
iccir marked this pull request as ready for review October 16, 2026 16:52
QuLogic reviewed Oct 16, 2026
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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
Fix indentation based on review
dc9df6a 
(plus a bonus tab-to-space fix)
QuLogic added the CI: Run cibuildwheel Run wheel building tests on a PR label Oct 16, 2026

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 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 approved these changes Oct 16, 2026
View reviewed changes

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 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 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 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 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 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?

github-actions Bot removed the CI: Run cibuildwheel Run wheel building tests on a PR label Oct 20, 2026
QuLogic merged commit bad0adf into matplotlib:main Oct 20, 2026
54 of 57 checks passed
QuLogic added this to the v3.12.0 milestone Oct 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

Assignees

No one assigned

Labels

backend: agg GUI: tk topic: contour topic: images topic: path handling topic: text/fonts

Projects

None yet

Milestone

v3.12.0

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL