Skip to content

Conversation

@user27182
Copy link
Contributor

Upstream as part of pyvista/pyvista#7716, setting dynamic attributes on meshes will no longer be allowed. MNE currently adds a _hemi attribute dynamically, which is causing integration test failures:
https://github.com/pyvista/pyvista/actions/runs/16335129476/job/46145621314?pr=7716#step:15:2380

I'm pretty sure they'll all be resolved by this PR with this change.

There will be a global flag as part of the API to allow setting dynamic attributes, but pending review/approval of pyvista/pyvista#7716, I'm pretty this will be off by default. Until a release is made with the "official" flag/mechanism for enabling this, a workaround for this restriction is to use object.__setattr__.

@user27182 user27182 changed the title Use __setattr__ to set PolyData._hemi dynamically Use __setattr__ to set PolyData._hemi dynamically Jul 17, 2025
# add metadata to the mesh for picking
mesh._polydata._hemi = h
# using __setattr__ is needed to set dynamic properties for PolyData
object.__setattr__(mesh._polydata, "_hemi", h)
Copy link
Contributor

Choose a reason for hiding this comment

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

can't this be setattr(mesh._polydata, "_hemi", h)? If that works, I would prefer to avoid calling a private dunder method.

Copy link
Contributor Author

@user27182 user27182 Jul 17, 2025

Choose a reason for hiding this comment

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

No it must use object.__setattr__ to work. The changes upstream explicitly forbids setting new attributes the "normal" way.

This change can be considered temporary in order to get our CI integration tests passing upstream. Once the change upstream is formalized and a new PyVista release is made, there will be a formal API for this, e.g.

- mesh._polydata._hemi = h
+ with pv.allow_new_attributes():
+    mesh._polydata._hemi = h

If you'd like I can add a TODO or open a new issue to track this. But in the interim this is needed for our MNE integration tests to pass.

Copy link
Contributor

@wmvanvliet wmvanvliet Jul 18, 2025

Choose a reason for hiding this comment

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

Thanks for the clarification. #13330 is a cleaner way to do this, but thanks for putting in the effort.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants