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

Use theme instead of several optional parameters for general_plotter #425

Open
1 of 3 tasks
natter1 opened this issue May 8, 2021 · 16 comments
Open
1 of 3 tasks

Comments

@natter1
Copy link
Contributor

natter1 commented May 8, 2021

Edit: Progress

It might be useful, to make second and third part into new issues.

EndEdit

Sorry for spamming issues, but as I play with the plot functionality (including pyvista, which is also new for me), I get a lot of ideas ^^
There are many parameters to change the look of plots, and also some missing parameters (e.g. to change label format). Even worse, by setting some of these to default values, its not possible to use pyvista themes like 'document' or 'night' without "removing" those default values (by setting parameters to ''). Lables can't be changed at all.

def plot_geometry(mapdl: MapdlCorba, theme="document"):
    pv.set_plot_theme(theme)
    plotter = pv.Plotter(shape=(2, 2))
    mapdl.nplot(plotter=plotter, color='')
    plotter.subplot(0, 1)
    mapdl.lplot(plotter=plotter, color='')#color_lines=True) # color="#000000")
    plotter.subplot(1, 0)
    mapdl.aplot(plotter=plotter, color='')
    plotter.subplot(1, 1)
    mapdl.eplot(plotter=plotter, color='')
    plotter.link_views()
    plotter.show(screenshot=True)
    plotter.screenshot(f'multiplot_example')

multiplot_example

I think it would be better to use a Theme to format most of the plot. It would be much cleaner and easier to use. Up till now I didn't find, how to use/add a custom theme with pyvista. But it seems like a simple dict to me - so it should be easy to add for pyvista, if it is really not implemented yet.

@akaszynski
Copy link
Collaborator

Sorry for spamming issues, but as I play with the plot functionality

No issue here. I appreciate the inputs because it's helping drive the direction. I can't guarantee I'll get back to all these issues quickly due to the volume of demands.

Up till now I didn't find, how to use/add a custom theme with pyvista. But it seems like a simple dict to me - so it should be easy to add for pyvista, if it is really not implemented yet.

Theme can be set with https://docs.pyvista.org/examples/02-plot/themes.html

I think it would be better to use a Theme to format most of the plot

Quite true. We do this a little bit by default with:
https://github.com/pyansys/pymapdl/blob/36598e484987308374e742833a31309243b010a4/ansys/mapdl/core/misc.py#L105-L112

Which is called on init of pymapdl. Changes need to be made module-wide to ensure that we're not overriding the theme with each method.

@natter1
Copy link
Contributor Author

natter1 commented May 11, 2021

Theme can be set with https://docs.pyvista.org/examples/02-plot/themes.html

I found that one. But I couldn't find anything to set a user defined theme. And I'm also not sure, which way would be the best to do it (at least I guess for now it doesn't matter whether its implemented in pyvista or pymapdl). Basicaly I see two options (there might be more):

  • Write a function similar to the one implementing the default themes, which accepts a dict as parameter (in fact this approach should be fairly easy to add to pv.set_plot_theme()) The disadvantage - imho it is not very user friendly. Which keys are allowed inside the dict? What can be set with each key? Also, it looks like there are a lot more keys to rcParams than are set with the default themes. Which ones are good to use? Esp. if some settings are not reverted when switching back to a default theme?
  • Create a class Theme, which provides an easy to understand interface for the user. The class could set the theme by itself or give a correct dict to pv.set_plot_theme(). A class could come with doc-strings, which is also a plus. When something has to change I would expect its easier to catch errors in legacy code compared to a pure dict, where some keys or values have become invalid.

Changes need to be made module-wide to ensure that we're not overriding the theme with each method

Thats also something I thought about. Its quite tricky. I see two different approaches here too.

  • do not set any theme-specific parameter inside the plot functions (which would block themes from doing what they are expected to do). If a user wants a certain theme, he has to set it before plotting. In many cases that might be the optimal solution, because its likly that all the plots in a script should use a similar theme. The disadvantage I see - its not possible to have different default behavior for e.g. a line-plot and an area plot.
  • set a new theme each time a plot is created (either a user given one or a default theme). In this case, the user has to be aware, that creating a plot might influence anything that is done afterwards. That might be ok if the user knows about it (and if he can easily save his settings in different theme objects/dicts). But it doesn't feel like a perfect solution.

If themes are implemented as a class, it might be worth to consider implementing a functionallity, which saves all the original rcParams-values before setting new ones and add a method restore_original()

Another alternative would be some kind of stylesheet-class, which only formats a given plot (without touching rcParams). That could be given as a parameter to a plot-function. If None, the plot-functions would fall back to the theme.

@akaszynski
Copy link
Collaborator

  • Create a class Theme, which provides an easy to understand interface for the user. The class could set the theme by itself or give a correct dict to pv.set_plot_theme(). A class could come with doc-strings, which is also a plus. When something has to change I would expect its easier to catch errors in legacy code compared to a pure dict, where some keys or values have become invalid.

Theme class seems preferable, something like:

class Theme():
    """Control the default plotting theme for ``pyvista``"""

    def __init__():
        self._background = [0.3, 0.3, 0.3]

    @property
    def background(self):
        """Default background color of a pyvista plot.

        Examples
        --------
        Set the default global background of all plots to white.

        >>> pyvista.theme.background = 'white'
        """
        return self._background

    @background.setter
    def background(self, new_background):
        self._background = parse_color(new_background)

This is preferable as we can incorporate some basic type checking when setting these values, rather than from a dictionary where the properties are not apparent in an IDE via code completion.

@natter1
Copy link
Contributor Author

natter1 commented May 14, 2021

@akaszynski I guess it will take a while until the new Theme-class becomes availeable in a new pyvista release, and afterwards in a new release of pymapdl. If you don't plan to implement it right away in pymapdl (I can't match your speed ^^), I would start to gather the different places where pymapdl is setting plot-format in a plotting_themes.py using the new pyvista themes over the weekend. Might help when deciding how to restructure pymapdl to use themes.

@akaszynski
Copy link
Collaborator

@akaszynski I guess it will take a while until the new Theme-class becomes availeable in a new pyvista release, and afterwards in a new release of pymapdl. If you don't plan to implement it right away in pymapdl (I can't match your speed ^^), I would start to gather the different places where pymapdl is setting plot-format in a plotting_themes.py using the new pyvista themes over the weekend. Might help when deciding how to restructure pymapdl to use themes.

I think this feature warrants a quick release as it's a critical feature for modules that rely on pyvista. I'll try to push one out soon, potentially over the weekend.

@natter1
Copy link
Contributor Author

natter1 commented May 14, 2021

I have looked into the default APDL plotting functions. Here is a summary plotting_themes.py

The short version:

class GeneralPlotterStyle(EmptyStyle):
    def __init__(self):
        super().__init__()
        self.title = ''
        self.background = None
        self.window_size = None
        self.notebook = None
        # add_mesh kwargs:
        self.color = 'w'
        self.show_edges = None
        self.edge_color = None
        self.lighting = None
        self.cmap = None
        self.render_points_as_spheres = False
        self.smooth_shading = None

        # parameters not part of pyvista.DefaultTheme()
        self.cpos = None
        self.show_bounds = False
        self.show_axes = True
        self.off_screen = None
        self.savfig = None
        self.point_size = 5.0
        self.line_width = None
        self.opacity = 1.0
        self.flip_scalars = False
        self.n_colors = 256
        self.interpolate_before_map = True
        self.render_lines_as_tubes = False
        self.stitle = None  # todo: stitle is deprecated in pyvista -> scalar_bar_args

        # todo: up till now this is to format labels - not the same as pyvista!
        self.font.size = None
        self.font.family = None
        self.text_color = None


class APlotStyle(EmptyStyle):
    # Type Hints
    vtk: Optional[bool]
    quality: Literal[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
    show_area_numbering: bool
    show_line_numbering: bool
    color_areas: bool
    show_lines: bool
    stitle: Optional[str]

    def __init__(self):
        super().__init__()
        self.title = "MAPDL Area Plot"

        # parameters not part of pyvista.DefaultTheme()
        self.vtk = None  # todo: what does None mean?
        self.quality = 4
        self.show_area_numbering = False
        self.show_line_numbering = False
        self.color_areas = False
        self.show_lines = False
        self.stitle = None  # -> where to put instead?


class EPlotStyle(EmptyStyle):
    show_node_numbering: bool
    vtk: Optional[bool]

    def __init__(self):
        super().__init__()
        self.title = 'MAPDL Element Plot'
        self.show_edges = True

        # parameters not part of pyvista.DefaultTheme()
        self.show_node_numbering = False
        self. vtk = None  # -> vtk = self._use_vtk


class KPlotStyle(EmptyStyle):
    vtk: Optional[bool]
    show_keypoint_numbering: bool

    def __init__(self):
        super().__init__()
        self.title = 'MAPDL Keypoint Plot'

        # parameters not part of pyvista.DefaultTheme()
        self.vtk = None  # -> vtk = self._use_vtk
        self.show_keypoint_numbering = True


class LPLOTStyle(EmptyStyle):
    vtk: Optional[bool]
    show_line_numbering: bool
    show_keypoint_numbering: bool
    color_lines: bool

    def __init__(self):
        super().__init__()
        self.title = 'MAPDL Line Plot'

        # parameters not part of pyvista.DefaultTheme()
        self.vtk = None  # -> vtk = self._use_vtk
        self.show_line_numbering = True
        self.show_keypoint_numbering = False
        self.color_lines = False


class NPLOTStyle(EmptyStyle):
    vtk: Optional[bool]

    def __init__(self):
        super().__init__()
        self.title = 'MAPDL Node Plot'

        # parameters not part of pyvista.DefaultTheme()
        self.vtk = None  # -> vtk = self._use_vtk


class VPLOTStyle(EmptyStyle):
    vtk: Optional[bool]
    quality: Literal[1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
    show_area_numbering: bool
    show_line_numbering: bool
    color_areas: bool
    show_lines: bool

    def __init__(self):
        super().__init__()
        # parameters not part of pyvista.DefaultTheme()
        self.vtk = None  # -> vtk = self._use_vtk
        self.quality = 4
        self.show_area_numbering = False
        self.show_line_numbering = False
        self.color_areas = False
        self.show_lines = True

There are a lot of pymapddl-specific parameters. The question here is, if we add this to a PyMAPDLTheme or add an addittional class-object or leave it as function/method-parameters.

@natter1
Copy link
Contributor Author

natter1 commented May 15, 2021

In post.py we have a lot of similar plot_nodal... methods. Theoretically we could replace them with a single method plot(solution: NodalSolution), where NodalSolution is an Enum-class. That would make testing, changing the interface and updating doc-strings a lot easier. I'm not so sure about the user-perspective. It would be neccessary to import and use an additional Enum.

@natter1
Copy link
Contributor Author

natter1 commented May 16, 2021

class NodalSolution(Enum):
    Displacement_X = auto()
    Displacement_Y = auto()
    Displacement_Z = auto()
    Displacement_NORM = auto()

    Rotation_X = auto()
    Rotation_Y = auto()
    Rotation_Z = auto()

    Temperature = auto()
    # ...


class _PostProcessing(PostProcessing):
    def __init__(self, mapdl):
        # todo: move to PostProcessing (allows function as self.nodal_displacement ...)
        super().__init__(mapdl)
        self._plot_dict = {  # scalar function, stitle, comp
            NodalSolution.Displacement_X: (self.nodal_displacement, 'X Displacement', 'X'),
            NodalSolution.Displacement_Y: (self.nodal_displacement, 'Y Displacement', 'Y'),
            NodalSolution.Displacement_Z: (self.nodal_displacement, 'Z Displacement', 'Z'),
            NodalSolution.Displacement_NORM: (self.nodal_displacement, 'NORM Displacement', 'NORM'),
            NodalSolution.Rotation_X: (self.nodal_rotation, 'X Rotation', 'X'),
            NodalSolution.Rotation_Y: (self.nodal_rotation, 'Y Rotation', 'y'),
            NodalSolution.Rotation_Z: (self.nodal_rotation, 'Z Rotation', 'Z'),
            NodalSolution.Temperature: (self.nodal_temperature, 'Nodal Temperature', None),
            # ...
        }

    def plot(self, solution: NodalSolution, show_node_numbering=False, **kwargs):
        func, _stitle, comp = self._plot_dict[solution]
        kwargs.setdefault('stitle', _stitle)
        if comp is not None:
            scalars = func(comp)
        else:
            scalars = func()

        return self._plot_point_scalars(scalars, show_node_numbering=show_node_numbering,
                                        **kwargs)

Even if we do not implement it this way, it might be a good testing solution without messing with the old interface. If this wotks as expected, its easy to transfer to the plot_nodal_...() methods.

@akaszynski
Copy link
Collaborator

In post.py we have a lot of similar plot_nodal... methods. Theoretically we could replace them with a single method plot(solution: NodalSolution), where NodalSolution is an Enum-class. That would make testing, changing the interface and updating doc-strings a lot easier. I'm not so sure about the user-perspective. It would be neccessary to import and use an additional Enum.

That needs to be implemented. I'm going to make a hefty PR to patch plotting along with using the theme Class just merged with pyvista. I'll do some basic testing and push a patch for 0.30.2 tomorrow.

@jgd10
Copy link
Contributor

jgd10 commented Jun 7, 2021

Can this issue be closed? It looks like it might have been resolved?

@akaszynski
Copy link
Collaborator

Can this issue be closed? It looks like it might have been resolved?

This needs pyvista/pyvista#1359 then a new PR that implements a theme.

@natter1
Copy link
Contributor Author

natter1 commented Jun 11, 2021

PR #477

@akaszynski
Copy link
Collaborator

discuss/add additional values for MapdlTheme class (numbering, etc.)

What additional kwargs/attributes do we need to add to MapdlTheme?

@natter1
Copy link
Contributor Author

natter1 commented Jun 16, 2021

What additional kwargs/attributes do we need to add to MapdlTheme?

I'm not sure if we need it (thats up for discussion imho). In the summary about plotting functions, I found many parameters that are not part of pyvista Theme, and might be added to MapdlTheme (or maybe not ^^).

        # general_plotter()
        # parameters not part of pyvista.DefaultTheme()
        self.cpos = None
        self.show_bounds = False
        self.show_axes = True
        self.off_screen = None
        self.savfig = None
        self.point_size = 5.0
        self.line_width = None
        self.opacity = 1.0
        self.flip_scalars = False
        self.n_colors = 256
        self.interpolate_before_map = True
        self.render_lines_as_tubes = False

        # todo: up till now this is to format labels - not the same as pyvista!
        self.font.size = None
        self.font.family = None
        self.text_color = None
        # aplot()
        # parameters not part of pyvista.DefaultTheme()
        self.vtk = None  # todo: what does None mean?
        self.quality = 4
        self.show_area_numbering = False
        self.show_line_numbering = False
        self.color_areas = False
        self.show_lines = False

Interesting might be e.g. show_bounds, show_axes, point_size, line_width, opacity, n_colors, render_lines_as_tubes, quality, show_line_numbering, show_area_numbering. Also, what about the label-format (e.g. size, color, backgroundcolor)?

@germa89
Copy link
Collaborator

germa89 commented Jul 7, 2022

This probably goes in line with making plotting an external library where the plotting is made by a class.

Or maybe we just need to call pyvista........

@germa89
Copy link
Collaborator

germa89 commented Mar 23, 2023

In #1938 we allow to pass more arguments to Pyvista object. It should alleviate the problem of customising plots.

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

No branches or pull requests

4 participants