Plotting/GUI CLI should they be separate or combined?

[zerothi]

We need to figure out whether we need two CLI for plotting and for GUI handling.

I am starting to more advocate one CLI for both.

sgui ase
sgui matplotlib/plot
sgui plotly

something like this.

Since we are in doubt of the placement of individual functionality, users will without a doubt also!! :)

Please chip in here! ;) @pfebrer96 @jonaslb

Originally posted by @zerothi in https://github.com/zerothi/sisl/pull/221#discussion_r487800251

[jonaslb]

I haven't really dived much into the visualization/plotting module of Pol, so my insight there is a bit limited. But in the end I think plotting graphs is quite different from viewing/editing geometries so in that sense I guess it would make sense to have them be separate clis.

Also it's true (Nick's comment) that the editing features in ase gui are very limited. I suppose it's more for getting a quick look at a structure in a gui that can give more or less all the geometry information one needs (which is quite often nice imo). Also it's pure python so it has no dependencies (other than ase).

[pfebrer]

But in the end I think plotting graphs is quite different from viewing/editing geometries so in that sense I guess it would make sense to have them be separate clis.

I have the same opinion. I would do splot struct.bands when I want to get a quick plot of a bands file, no need for a GUI. In the current implementation of splot in the viz module you can also pass the settings as --arguments and even use the presets that you have previously defined for yourself, as well as saving the plot or writing it to an image. So I would say that is about all the functionality you would need for just getting quick plots without the overhead of a GUI (quick doesn't mean simple and bad, they can be as beautiful and complex as the presets you have defined).

[zerothi]

But what about end-users who don't really know the difference?

I am a bit worried that users will confuse the two.
Could we perhaps have a middle ground sdisplay or sview, or something similar? Since all the functionality is passed down to sub-parsers I don't think we need two cli's.

My point being that while matplotlib plots are minor, they still do provide some level of interaction (change of limits etc.) so in that respect it may be considered a GUI.

Would you be very much against having sgui as the cli for everything?

[pfebrer]

Ok, I understand that it is hard to objectively draw a hard line between what is a GUI and what is not. The main problem is probably that we leave in a very nice era where plots themselves are interactive :)

The problem with sdisplay is that I don't see how a user would expect sdisplay to open a GUI that manages multiple plots and tabs (in the case of the viz.plotly GUI).

Probably one of the problems here is that we are trying to unify a very diverse set of functionalities in the case of the GUIs. For plots, splot or sdisplay is good for all engines of plotting, since in the end they all do the same but using different software. On the other hand, the viz.plotly GUI is focused on allowing you to manage multiple plots given the files that you have in your filesystem, while the ase gui focuses on modifying structures and the (future) blender gui will also focus mainly on modifying structures (and may have other features).

[zerothi]

Ok, I understand that it is hard to objectively draw a hard line between what is a GUI and what is not. The main problem is probably that we leave in a very nice era where plots themselves are interactive :)

The problem with sdisplay is that I don't see how a user would expect sdisplay to open a GUI that manages multiple plots and tabs (in the case of the viz.plotly GUI).

Probably one of the problems here is that we are trying to unify a very diverse set of functionalities in the case of the GUIs. For plots, splot or sdisplay is good for all engines of plotting, since in the end they all do the same but using different software. On the other hand, the viz.plotly GUI is focused on allowing you to manage multiple plots given the files that you have in your filesystem, while the ase gui focuses on modifying structures and the (future) blender gui will also focus mainly on modifying structures (and may have other features).

This makes me think that sgui should be used for everything ;)

[zerothi]

Alternatively the command sisl could be used to dispatch any functionality?

sisl geometry/geom siesta.XV siesta.fdf
sisl plotly RUN.fdf
sisl blender Rho.grid.nc
sisl ase RUN.fdf

or the likes?

[pfebrer]

By the way:

Would you be very much against having sgui as the cli for everything?

In my opinion it is not intuitive that sgui struct.bands will give you a bands plot. Maybe you could sgui plot struct.bands, but then you are making the process of quick plotting more complicated.

I would say that the main difference between gui and plot is in the timeframes (as weird as it sounds). One should be prepared to use much more of their time when they type sgui than when they type splot.

[zerothi]

Would you be very much against having sgui as the cli for everything?

In my opinion it is not intuitive that sgui struct.bands will give you a bands plot. Maybe you could sgui plot struct.bands, but then you are making the process of quick plotting more complicated.

In this case users would have to do sgui plot struct.bands Agreed that sgui struct.bands shouldn't be favoured.

I would say that the main difference between gui and plot is in the timeframes (as weird as it sounds). One should be prepared to use much more of their time when they type sgui than when they type splot.

hmm, users could just do alias splot='sgui plot' and then not need to worry about anything? I am just more concerned that adding too many CLI's is confusing, if sgui plot is used alot, there are easy ways around that. Having this described in the documentation for the sgui would be absolutely fine to show how easy it is.

[pfebrer]

hmm, users could just do alias splot='sgui plot' and then not need to worry about anything? I am just more concerned that adding too many CLI's is confusing, if sgui plot is used alot, there are easy ways around that. Having this described in the documentation for the sgui would be absolutely fine to show how easy it is.

Ok, maybe we could provide a way to switch on/off the shortcuts (e.g. splot, sgeom) within sisl so that users don't need to do it themselves? Believe it or not, not everyone knows that they can define an alias or where to define it. And this is even more difficult to handle if sisl is inside an environment and you need to have the commands only when the environment is activated. Would it be possible to handle this from sisl?

[zerothi]

Ok, maybe we could provide a way to switch on/off the shortcuts (e.g. splot, sgeom) within sisl so that users don't need to do it themselves? Believe it or not, not everyone knows that they can define an alias or where to define it. And this is even more difficult to handle if sisl is inside an environment and you need to have the commands only when the environment is activated. Would it be possible to handle this from sisl?

Hmm... how should we then decide whether they should be present or not. Perhaps it would be ok to have sgui and splot where splot is just sgui matplotlib/plot?

[pfebrer]

I don't know, I think this could be up to the users. By default they could be all turned off. And just something like sisl.create_cli_shortcut("splot", "sisl plot"). So in reality it would be just allowing users to create and manage aliases without having to know how to do that in the environment. Is it possible to do this?

[pfebrer]

The main problem is that users will not know how to set aliases in the environment and therefore most won't do it. (I don't know how to do it)

[pfebrer]

I don't know, I think this could be up to the users.

I retract myself, it's probably better to have consistent shortcuts accross sisl instalations. Sorry hahaha

[zerothi]

Hmm, we could do that, it would requires us to hook into the setuptools method for creating cli's I think.

Hmm, I am not a fan of this (lots of work for very little).

To create an alias, simply do this:

echo "alias splot='sgui plot'" >> $HOME/.bashrc

and done ;) next time you have a shell, you have this command.

For instance I have a shorthand:

alias cds='cd $HOME/path/to/sisl'

This lets me do cds anywhere and easily go to the sisl code repository.

[pfebrer]

Yes, what I mean is how to activate/deactivate aliases depending on the python environment you are in. I.e. I only want splot if I'm in my sisl environment.

[zerothi]

Yes, what I mean is how to activate/deactivate aliases depending on the python environment you are in. I.e. I only want splot if I'm in my sisl environment.

Hmm, you mean a virtual environment? No I don't think we should go into this sphere. It would again confuse since some users would have the cli's whereas others may not.

[zerothi]

I think I have made up my mind ;)
We will go for this:

sisl plot [--backend matplotlib|blender|plotly|...]
sisl gui [--backend plotly|blender|matplotlib|...]

I think that sisl plot|gui should allow the same backends but that they both will default to different backends.
End-users may not exactly know the difference and then I think this is better...

Any comments? @pfebrer

[pfebrer]

:+1:

I think it would be great to unify the viz module so that plotting backends are only that: plotting backends. The rest of the processing and methods can be shared and then it's only a choice of how do you want to display it.

I have already give quite some thought to this and I think it would be relatively easy to incorporate into the already existing code. Using a decorator could be a clean approach. The plotting process would be separated into three very independent steps:

The steps where there are multiple ways to do things (reading input, rendering display) would use a decorator to give the specific details. As a quick snippet (entry points are already implemented):

class BandsPlot(Plot):
      # Entry points
      @entry_point("siesta output")
      def _read(...):
           self.bands = ...read from siesta input
      
      @entry_point("hamiltonian")
      def _read(...):
           self.bands = ...generate bands from hamiltonian

      # Common processing
      def common_processing(...):
           ...do whatever common thing with self.bands according to the settings
          # Call the drawing method, which will call one of the defined `draw_bands` depending
          # on the selected backend
          self.draw_bands(...)

      # Plotting backends
      @backend("plotly")
      def draw_bands(...):
            ...draws bands with plotly

      @backend("matplotlib")
      def draw_bands(...):
            ...draws bands with matplotlib

I can give it a go using the plots that you already created using matplotlib.

[zerothi]

Ok, so now my head is starting to wrap around all this :) This is great! Also see #292.

We should make them very agnostic about what to do:

So something like

class Plot:

    def __init__(self, obj):
        self.obj = obj # could be a sile, or a Hamiltonian or

    @abstractmethod
    def prepare(self, *args, **kwargs):
         """ Prepare plots, could be reading data, or creating a figure or ..."""
    @abstractmethod
    def draw(self, *args, **kwargs):
 ...

I kind of like the methodology for Dispatchers, i.e. by registering dispatchers. That would mean more power to the people and easier to add verbose backends through sub-modules.

What do you think about this?

[pfebrer]

I'm curious, what's the difference between creating a figure in the prepare method and the draw method?

This is almost the same as I have now, with different naming. prepare is the current read_data and draw is the current set_data. Only that set_data also does some processing before actually drawing.

Your naming seems fine, and dispatchers are a great idea! Much cleaner than wrappers.

Two comments:

• Explicitly separating the read_data part from the rest was deliberate, I didn't want to be reading the data from files if it was not needed because of: (1) files might be read from remotes through slow connections -this is mitigated by the cache, but the cache is not infinite- and (2) once you save the plot, when you load it the file might not be available anymore. So you should be able to still modify the plot if there's no need to read new data.
• I think that dividing it in three steps is better (entry -> prepare -> draw). Mainly because of the above point. Entry points should do the bare minimum to load the data and ensure after them everything can be done regardless of the entry point. So in my scheme, entry and draw would be dispatchers, but prepare would not.

[zerothi]

I'm curious, what's the difference between creating a figure in the prepare method and the draw method?

This is almost the same as I have now, with different naming. prepare is the current read_data and draw is the current set_data. Only that set_data also does some processing before actually drawing.

Exactly, it is to clarify order of execution. In your case it isn't exactly clear which order things should be done in. I think my naming is a bit clearer. I don't really mind the wording, I would just like it to be clear which order they are intended.

Your naming seems fine, and dispatchers are a great idea! Much cleaner than wrappers.

Two comments:

• Explicitly separating the read_data part from the rest was deliberate, I didn't want to be reading the data from files if it was not needed because of: (1) files might be read from remotes through slow connections -this is mitigated by the cache, but the cache is not infinite- and (2) once you save the plot, when you load it the file might not be available anymore. So you should be able to still modify the plot if there's no need to read new data.
• I think that dividing it in three steps is better (entry -> prepare -> draw). Mainly because of the above point. Entry points should do the bare minimum to load the data and ensure after them everything can be done regardless of the entry point. So in my scheme, entry and draw would be dispatchers, but prepare would not.

Ok, that would be fine. May I suggest we add a 4th item. entry -> prepare -> draw -> postprocess (or deprepare or something)

Sometimes it may be nice to close files, delete temporary stuff or something else. :)

[pfebrer]

entry -> prepare -> draw -> postprocess (or deprepare or something)

Sometimes it may be nice to close files, delete temporary stuff or something else. :)

Hahah ok, I thought siles automatically closed the file handle after you read.

Exactly, it is to clarify order of execution. In your case it isn't exactly clear which order things should be done in. I think my naming is a bit clearer. I don't really mind the wording, I would just like it to be clear which order they are intended.

My question was about the doc line of prepare:

""" Prepare plots, could be reading data, or creating a figure or ..."""

What does it mean "creating a figure" here?

[pfebrer]

Ok, that would be fine. May I suggest we add a 4th item. entry -> prepare -> draw -> postprocess (or deprepare or something)

Sometimes it may be nice to close files, delete temporary stuff or something else. :)

But the files that you have open will depend on the entry point, so why not just close them when you are not going to use them anymore instead that at the end? Anyway, a general method at the end is useful. In fact, right now I have callbacks when certain points of the process are reached that plots can use if they need to. So for read_data there is _before_read and _after_read,and after the figure is drawn there is _after_get_figure.

Maybe we don't need to create a full step, this kind of callbacks could be enough. In fact instead of with methods we could also handle this with a dispatcher (where registering a dispatcher in this case would be somewhat similar as adding event listeners in javascript). Then you can trigger events like self.event_dispatcher("figure_drawn", ...). This would be extensible to clicks and other kind of events, which I already thought about in this way.

[zerothi]

entry -> prepare -> draw -> postprocess (or deprepare or something) Sometimes it may be nice to close files, delete temporary stuff or something else. :)

Hahah ok, I thought siles automatically closed the file handle after you read.

Yeah, most do, but some (the fortran binary files) needs to be closed somewhat manually (in some cases).

Exactly, it is to clarify order of execution. In your case it isn't exactly clear which order things should be done in. I think my naming is a bit clearer. I don't really mind the wording, I would just like it to be clear which order they are intended.

My question was about the doc line of prepare:

""" Prepare plots, could be reading data, or creating a figure or ..."""

What does it mean "creating a figure" here?

Yeah, I don't know ;)

[pfebrer]

I'm preparing the plotting part of the CLI, but since you wanted to put everything under the sisl command, do you want to reorganize everything to create the command and then I will add the plotting part?

[zerothi]

I think you can create what is needed behind the scenes something that gets the ArgumentParser.

On the other hand I have thought about moving away from argparse, something like Click. I have encountered some problems with argparse that seem unnatural on the cli. Do you have any experience with anything else than argparse?

[zerothi]

I don't know if I should try and change to Click on a separate branch, then we can decide after this? Or do you think that your command isn't too big?

[pfebrer]

The plotting command is already created using argparse, so I just need to integrate it into the command you create. It is very simple compared to the other commands in sisl (although with time it could increase its complexity).

I don't have any experience with any argument parser other than argparse. It is true that I have encountered some problems with it, mainly some structures being more complex to generate than I initially thought, but I could finally made it work. If you think Click is better, I'm willing to change it. However, argparse seems to have less potential for installation/compatibility problems since it's part of the standard library, I don't know :)

[zerothi]

I don't have any experience with any argument parser other than argparse. It is true that I have encountered some problems with it, mainly some structures being more complex to generate than I initially thought, but I could finally made it work. If you think Click is better, I'm willing to change it. However, argparse seems to have less potential for installation/compatibility problems since it's part of the standard library, I don't know :)

Agreed. It is just really difficult with argparse, it does some parsing that is not desirable which requires manual hooks. And Click seems to have gotten much attraction and thus probably widely installed. Lets stick with argparse for some time, and when we can't do anything else, we'll probably have to adapt a new machinery!

[zerothi]

If you start with all the stuff behind the scenes i.e. only stuff in sisl.viz directory. Then I can aid in adding the main functionality, also so I can understand it ;)