cherab / core

The core source repository for the Cherab project.
https://www.cherab.info
Other
44 stars 24 forks source link

ToroidalVoxelGrid.plot() raises a TypeError with Matplotlib 3.8 #422

Closed vsnever closed 9 months ago

vsnever commented 10 months ago

Passing the closed parameter to Polygon.__init__() positionally, like here: https://github.com/cherab/core/blob/089d88a046be96f16a15e29e90e723a4b742c615/cherab/tools/inversions/voxels.pyx#L684-L686 was deprecated in Matplotlib 3.6 and in Matplotlib 3.8 raises a TypeError.

Mateasek commented 10 months ago

The issues we are getting because of Matplotlib are getting quite annoying. Generally I think that the classes should not have plotting methods. A better approach in my opinion is to plot in functions external to cherab classes, which access the class data. What do you think? I know that changing this now would break backwards compatibility, but if the suggested approach is accepted, we can have it as a milestone for the next major version of Cherab.

vsnever commented 10 months ago

The issues we are getting because of Matplotlib are getting quite annoying. Generally I think that the classes should not have plotting methods. A better approach in my opinion is to plot in functions external to cherab classes, which access the class data. What do you think? I know that changing this now would break backwards compatibility, but if the suggested approach is accepted, we can have it as a milestone for the next major version of Cherab.

I agree, this will also help improve class design by ensuring that if a protected/private attribute is used for plotting, it should be available as a property.

In addition to ToroidalVoxelGrid the plotting method is also present in the _PECRate class, where it definitely shouldn’t be. We also have plotting methods in the mesh classes in cherab-solps, cherab-edge2d and cherab-imas.

For now, I'll just merge the fix from #423 so the tests can be passed.

vsnever commented 9 months ago

Fixed in #423.