Figure: Refactor how plotting methods are attached to the Figure class#4612
Figure: Refactor how plotting methods are attached to the Figure class#4612
Conversation
|
This PR is backward-compatible, but it's a big low-level refactor. Ping @GenericMappingTools/pygmt-maintainers for reviews. Edit: Previously, it was possible to do |
|
This PR also renames the |
| wiggle, | ||
| ) | ||
| # Attach plotting functions implemented in pygmt.src as Figure methods. | ||
| basemap = _basemap |
There was a problem hiding this comment.
Couldn't we just directly do
| basemap = _basemap | |
| from pygmt.src.basemap import basemap |
So that the _basemap is not needed?
There was a problem hiding this comment.
Ah ok, if we did this, we would still need to handle:
error: Unsupported class scoped import [misc]
PLC0415 `import` should be at the top-level of a file
Co-authored-by: Yvonne Fröhlich <94163266+yvonnefroehlich@users.noreply.github.com>
| # Attach plotting functions implemented in pygmt.src as Figure methods. | ||
| basemap = _basemap |
There was a problem hiding this comment.
Can these go into the __init__ method? Might need to rewrite as self.basemap = _basemap?
There was a problem hiding this comment.
Oo, doesn't actually work. Getting TypeError("coast() missing 1 required positional argument: 'self'") 😅
There was a problem hiding this comment.
Maybe revert that commit. We actually do want the figure methods as class variables, not instance variables (https://docs.python.org/3/tutorial/classes.html#class-and-instance-variables).
weiji14
left a comment
There was a problem hiding this comment.
Have some ideas using importlib or Mixins (following ideas in https://stackoverflow.com/questions/47561840/how-can-i-separate-the-functions-of-a-class-into-multiple-files), but the diff might be even bigger, so will just settle with this for now. Thanks @seisman.
Motivation
In PR #3849, we're seeing many static type check errors (https://github.com/GenericMappingTools/pygmt/actions/runs/25118862756/job/73613589767?pr=3849), mainly because in
pygmtlogo, we're trying to re-import theFigureclass, which causes some circular imports.Codex recommends a different way to attach the plotting wrappers to the
Figureclass, which can solve the static type error found in PR #3849. Changes in this PR are made by Codex.Summary
This PR refactors how plotting methods are attached to
pygmt.Figureand clarifies the role ofpygmt.src.Why
Previously,
Figuremethods were attached through a class-scopedfrom pygmt.src import (...)block inpygmt/figure.py. That worked at runtime, but it had a few drawbacks:pygmt/src/__init__.pyjust to populateFigure.FigureWhat changed
pygmt/figure.pypygmt.src.*module.Figureclass body, e.g.plot = _plot.pygmt/src/__init__.pyFigure-bound plotting methods frompygmt.src.Figuremethods.Why this is better
This makes the API structure clearer:
pygmt.srcbecomes a cleaner namespace for standalone operations.Figureis the clear home for plotting methods.It also improves maintainability:
Figuremethod points to one explicit source module.Figureno longer depends on package-level re-export behavior for method attachment.Behavioral intent
This is intended as an internal API-structure cleanup, not a plotting behavior change. The plotting implementations themselves are unchanged; only the way they are attached/exported has been reorganized.