GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Use mpl::format for error messages instead of pybind11 by iccir · Pull Request #32484 · matplotlib/matplotlib

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@github-actions github-actions Bot added GUI: tk backend: agg topic: path handling topic: images topic: text/fonts topic: contour labels Oct 16, 2026
@iccir
iccir marked this pull request as draft October 16, 2026 16:36
@iccir
Add mpl::format and cleanup mplutils
27a1c1e
@iccir
iccir force-pushed the mplutils branch from 6464f38 to 27a1c1e Compare October 16, 2026 16:40
@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

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/_image_wrapper.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/_image_wrapper.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/_tkagg.cpp Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/mplutils.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
@iccir
Fix indentation based on review
dc9df6a 
(plus a bonus tab-to-space fix)
QuLogic
QuLogic approved these changes Oct 16, 2026
View reviewed changes
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
story645
story645 approved these changes Oct 16, 2026
View reviewed changes

@story645 story645 left a comment

Copy link
Copy Markdown
Member

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 like the namespacing, am concerned about the coverage (but also it could be a case of picking up stuff that wasn't ever covered)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
tacaswell
tacaswell approved these changes Oct 16, 2026
View reviewed changes
@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 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the CI: Run cibuildwheel Run wheel building tests on a PR label Oct 20, 2026
@QuLogic
QuLogic merged commit bad0adf into matplotlib:main Oct 20, 2026
54 of 57 checks passed

Uh oh!

There was an error while loading. Please reload this page.
@QuLogic QuLogic added this to the v3.12.0 milestone Oct 20, 2026
@iccir iccir mentioned this pull request Oct 30, 2026
nanobind Umbrella #32560
Open
@github-project-automation github-project-automation Bot moved this to Todo in nanobind Nov 2, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in nanobind Nov 2, 2026
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

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

Projects

nanobind
Status: Done

Milestone

v3.12.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@iccir @story645 @tacaswell @QuLogic
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.