Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Internal refactor of figures and views #231

Merged
merged 13 commits into from
Aug 4, 2023
Merged

Internal refactor of figures and views #231

merged 13 commits into from
Aug 4, 2023

Conversation

nvaytet
Copy link
Member

@nvaytet nvaytet commented Aug 2, 2023

Internally, it has gotten quite confusing what is a Figure and what is a View.
Figure currently refers to multiple things:

  • the object returned by the plot function (and similar) which contains another "figure" inside, and maybe a toolbar also
  • the "figure" mentioned above which could be a FigLine or a FigImage, which contains a canvas
  • the underlying matplotlib figure, contained in the FigLine's canvas.

This is getting difficult to navigate.

This PR proposes to move things around and rename for (hopefully) improved clarity:

  • the FigLine and FigImage and now called views: LineView & ImageView
  • the object returned by the plot function is still a figure. It inherits methods from the common Figure mixin, which in turn inherits from the BaseFig (now just a mixin class)
  • the underlying matplotlib figure is still called fig

@nvaytet nvaytet marked this pull request as draft August 2, 2023 20:42
@nvaytet nvaytet marked this pull request as ready for review August 3, 2023 11:11
@@ -98,7 +98,7 @@ def is_sphinx_build() -> bool:

def copy_figure(fig: FigureLike, **kwargs) -> FigureLike:
out = fig.__class__(
fig._fig_constructor,
fig._view.__class__,
Copy link
Member

Choose a reason for hiding this comment

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

In case we are not keeping the callable anymore,
should we make sure if the _view.__class__ is the View in def __init_figure__(self, View, *args, **kwargs):...?

For example,

class FigBase:
  def __init_figure__(self, View, *args, **kwargs):
    self._view = View(*args, **kwargs)
    if not self._view.__class__ is View:
        raise TypeError

Cause, you might have something like

@dataclasses
class Figure:
  width: int
  height: int

def create_figure(size) -> Figure:
  return Figure(size[0], size[1])

fig = InteractiveFigure(create_figure, size=(10, 10))

In this case above, it will create the fig as intended, but copy_figure will not work.

Or is it too much of worry...?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think any user should every be constructing InteractiveFig manually, so I would say in this case it is not worth worrying about?

@nvaytet nvaytet merged commit d5f7537 into main Aug 4, 2023
3 checks passed
@nvaytet nvaytet deleted the to_dict branch August 4, 2023 18:02
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