ENH: Detaching plotly from the processing and allowing other drawers. #312

pfebrer · 2021-03-09T20:51:17Z

First of all, sorry to give you more work to review :)

Second: The idea is actually working and I'm excited to have this implemented!

PR introduction

Basically, as we were discussing in #266 (comment) and the following comments, this PR is a first sketch to have three different steps (not necessarily with these names):

Entry: There are different entries that read from different sources. They all normalize the data for the next stage. Already implemented.
Processing: Processes the data according to settings (which can be updated) to generate the data ready to plot.
Draw: Different drawers can be implemented for the same plot.

The entry and draw phases would allow the user to register new methods.

Here is the diagram that I draw initially to explain the concept:

In this PR, the separation of the drawing parts from the rest is done so that we can have this distinction.

What has been done

I have removed everything related to plotly from the main files.
I have moved all the plots to sisl.viz
I created a new submodule sisl.viz.drawers where the different drawers are implemented. Right now you can find a folder for plotly and another for matplotlib.
There is a file sisl/viz/drawers/plotly/drawer.py that contains the generic things for plotly figures. Same for matplotlib.
sisl/viz/drawers/plotly/drawers/* contains the specific drawers for each plot (right now only bands), which are then registered to the corresponding plotting class (in the same file). Same for matplotlib.
When a subclass of Plot is defined (i.e. in Plot.__init_subclass__), it receives a drawers manager, which will be waiting for new drawers to be registered. This drawers manager is defined in sisl.viz.drawers._plot_drawers. I just did it as simple as I could, waiting for you to reimplement it as you find suitable.
Hasn't been done but needs to be done: The PlotEngine in sisl.viz._plotables doesn't make sense anymore because drawer is just a regular setting that you can update on the fly.

The only specific plot to which the changes have been implemented for now is the bands plot. This is because I wanted to have a proof of concept so that you can give your opinion and propose different approaches. After we agree with what's the best approach, I can do it with the rest of plots. Presumably it is a significant amount of work so it's better to first have a consensus :)

How it works

import sisl
# Import the visualization module
import sisl.viz
# Import the drawers that you want, otherwise they are not loaded
# This could be somehow automatized with environment variables
import sisl.viz.drawers.plotly
import sisl.viz.drawers.matplotlib

# The plot is drawn with the default drawer (plotly for now)
plot = sisl.get_sile("myfile.bands").plot()
plot.show()

plot.update_settings(drawer="matplotlib")
plot.show()

The settings of BandsPlot of course work regardless of the drawer. If you want to render subplots, this is also drawer-aware:

# Draw some subplots
plot = sisl.get_sile("myfile.bands").plot(
    bands_color=["red", "orange", "green", "purple"], gap=[True, False, False, True], subplots=["bands_color", "gap"],
    gap_color="black", rows=2,
)
plot.show()

plot = sisl.get_sile("myfile.bands").plot(
    bands_color=["red", "orange", "green", "purple"], gap=[True, False, False, True], subplots=["bands_color", "gap"],
    gap_color="black", rows=2, drawer="matplotlib"
)

By the way: I have tried to uninstall plotly and the bands work with the matplotlib drawer :)

Looking forward to your feedback!

zerothi · 2021-03-10T07:42:02Z

Looks amazing! Thanks!

Yes, more work!! ;)
I'll probably have to post-pone this until after TS workshop, time is limited ;)
But just a comment, could we name it backend instead of drawer? I am imagining this could be useful for other stuff as well :)

And great you only started with one. Then we can have an iteration there!

pfebrer · 2021-03-10T12:30:27Z

But just a comment, could we name it backend instead of drawer?

I was thinking about that but since you can write as many different drawers as you want that use the same plotting framework for a given plot, backend seems too restrictive because it seems to be synonim of plotting framework in this case.

So backend seemed both too generic in the general picture (data processing can also be seen as a backend) and too restrictive in the picture of plotting (might give the idea that the mapping "plotting framework -> backend" should be one to one, and it's not the case). That's why I chose not to use it.

I am imagining this could be useful for other stuff as well :)

What do you have in mind? 🤔

zerothi · 2021-03-11T14:33:54Z

But just a comment, could we name it backend instead of drawer?

I was thinking about that but since you can write as many different drawers as you want that use the same plotting framework for a given plot, backend seems too restrictive because it seems to be synonim of plotting framework in this case.

True, but you would still have to name the drawer differently for each of them, d1 vs d2 etc.

So backend seemed both too generic in the general picture (data processing can also be seen as a backend) and too restrictive in the picture of plotting (might give the idea that the mapping "plotting framework -> backend" should be one to one, and it's not the case). That's why I chose not to use it.

I am imagining this could be useful for other stuff as well :)

What do you have in mind?

For exactly the general picture you mention, there is no need to make users think this is restricted to drawing stuff.
Some drawers may not have a direct API back end and one would have to resort to temporary files + shell commands. While that may be interpreted as a drawer, I would still consider it a backend.

I kind of like the generality here. It is what matplotlib uses as words for different frameworks. So it is also a matter of choosing a common word. So I have to say, backend would be the proper word :)

pfebrer · 2021-06-01T08:43:38Z

Hey Nick, do you think we can work on this before the SIESTA school? I would like to present it there 😅

zerothi

Yes, i think we should finish this.

However, we still have to decide on "backend" name. I would still like the name to be backend.

zerothi · 2021-06-01T10:57:25Z

sisl/viz/_plotly_plotables.py

+#     register(GeomSile, GeometryPlot, 'geom_file', default=True)
+#     register(GeomSile, BondLengthMap, 'geom_file')
+
+for HSile in get_siles(attrs=["read_hamiltonian"]):


if you don't use __all__ then you should prefix local variables with _ to make them private.

zerothi · 2021-06-01T11:07:14Z

Could you also rebase?

pfebrer · 2021-06-01T16:42:01Z

Great, thank youuuu! I will rename it to backend.

pfebrer · 2021-06-01T16:53:23Z

Now the path to a given drawer is sisl/viz/drawers/plotly/drawers/bands.py.

I understand that the first "drawers" could be replaced by backends, but the second one?

Also how would you rename the classes? MatplotlibBandsDrawer -> MatplotlibBandsBackend?

zerothi

I'll probably have some more stuff. But this drawer vs. backend seems like the most pressing thing?

sisl/viz/drawers/matplotlib/__init__.py

sisl/viz/drawers/_plot_drawers.py

sisl/viz/drawers/matplotlib/drawers/__init__.py

pfebrer · 2021-06-15T08:25:06Z

Ok, the renaming is done. The next things are:

how should the sisl-provided backends be loaded. Should we just load them if the needed packages are there? I.e. if matplotlib is available, matplotlib backends are loaded on import sisl.viz.
do you think it's a good idea to have abstract classes to define the methods that each particular backend should have?. E.g. an abstract BandsBackend that bands backends have to inherit from.

zerothi · 2021-06-15T11:19:35Z

how should the sisl-provided backends be loaded. Should we just load them if the needed packages are there? I.e. if matplotlib is available, matplotlib backends are loaded on import sisl.viz.

I think we shouldn't load unless a user wants too do it, i.e. lazy load when doing plot(object) and/or manual imports.
The reason is that sisl may be used for post-processing and loading GUI's is a waste of time.
Better be safe than sorry.

do you think it's a good idea to have abstract classes to define the methods that each particular backend should have?. E.g. an abstract BandsBackend that bands backends have to inherit from.

Definitely!!! :)

pfebrer · 2021-06-15T12:13:09Z

I'm not saying to load them on import sisl, I'm saying to load them on import sisl.viz. I think at that point is safe to assume that they want the visualization features and half a second is not a significant time

pfebrer · 2021-06-15T12:15:42Z

I propose that at that point everything is loaded by default and if you want you can block the auto loading with env variables. But I think the average user would want everything to be loaded at that point without any extra configuration.

zerothi · 2021-06-15T12:26:31Z

I'm not saying to load them on import sisl, I'm saying to load them on import sisl.viz. I think at that point is safe to assume that they want the visualization features and half a second is not a significant time

This is fine for me.

zerothi · 2021-06-15T18:34:07Z

@pfebrer could you unmark "draft"

pfebrer · 2021-06-15T22:50:50Z

By the way, in this PR or a subsequent one I would like to finally decide what the CLI for plotting should look like, now that everything will be unified.

zerothi · 2021-06-16T06:43:08Z

By the way, in this PR or a subsequent one I would like to finally decide what the CLI for plotting should look like, now that everything will be unified.

See here I think we should go for sisl plot/gui

zerothi · 2021-06-16T06:43:22Z

In a subsequent one! :)

pfebrer · 2021-06-16T11:22:15Z

Ok, I added a backends/templates directory that basically contains all the abstract classes.

Templates are registered on the backends manager with the register_template method. When you attempt to register a backend that doesn't inherit from the template an error is raised.

Any suggestion/comments about the organization and implementation of this?

zerothi · 2021-08-12T19:37:11Z

@pfebrer I have just rebased, could you please hold your branch for now (locally). There were no conflicts, but I am trying to fix the warnings!

zerothi · 2021-08-12T20:02:40Z

When I run the tests I get lots of "print-out errors", but not formal errors?

viz/plots/tests/test_plots.py::TestPlotSubClass::test_save_and_load[PdosPlot]
  info:0: SislInfo:
  
  The plot has been initialized correctly, but the current settings were not enough to generate the figure.
  Error: Could not read or generate data for PdosPlot from any of the possible sources.
  Here are the errors for each source:
  	- TB trans: TypeError.expected str, bytes or os.PathLike object, not NoneType
  	- hamiltonian: TypeError.expected str, bytes or os.PathLike object, not NoneType
  	- siesta output: TypeError.expected str, bytes or os.PathLike object, not NoneType

is this on purpose, or should this test actually have failed?

zerothi · 2021-08-12T20:15:50Z

sisl/viz/plots/geometry.py

+            otherwise they are taken from the geometry. 
+        axis: {0,1,2, "x", "y", "z", "a", "b", "c"} or array-like of shape 3, optional
+            the direction to be displayed along the X axis. 
+            If it's an int, it will interpreted as the index of the cell axis.


In your _sanitize_axis routine you interpret (0, 1, 2) as Cartesian coordinates?
I was confused whether cell meant lattice or Cartesian cell?
Please clarify?

This is an old docstring that should have been changed. Initially 0 meant the first lattice vector, but since I realised that it was strange that users needed to project along a lattice vector I decided to change it. Now, as you understood, 0 is the same as "x" as it's always more comfortable to write integers than to write strings. Any opinion on whether this is fine or not (it may be inconsistent with other parts of sisl, for example)?

My other implementation here sisl/utils/misc.py:direction could solve this kind of easily. I would suspect integer indices to respect lattice vectors. Typing an x vs. a 0 is not a problem I think

Hmm ok, the problem is that it's not an x, it's an "x" :) And when you do it lots of times it becomes annoying to write the ".

I would suspect integer indices to respect lattice vectors.

Yeah me too, that's why this was the initial idea. But for the representations I can't see many cases where you would want to project the lattice vectors into the plot axes to visualize real space data.

But isn't this in the GUI drop-down? I think "x" is fine to force people to do. It makes the code readable. Integers are not really good either. Better to force good habits than remove 2 characters. :)

Perhaps now there isn't much reason, but perhaps later...

Hmm maybe the issues that I have could be solved by allowing multicharacter strings also? I mean axes="xyz" would be the same as axes=["x", "y", "z"]. I think my speed thrive would be satisfied with that :) hahahah

So it should be easy to use, not to be fast.

I understand this, but if the point of the software is to allow quick visualization, then I think "quick to use" is also a feature that one would expect.

I really don't think users are in this category ;)
Only very high level users will be irritated about this.

I'm worried that people will just use 0,1,2 as shortcuts for "x", "y", "z" when they have an (almost) orthogonal cell, not because they want the projection of the axes, but because they want the cartesian representation and it's more convenient to write integers. This will then not behave in the same way when the cell is not orthogonal.

That is fine, there has to be rules. And you and I both thought that the lattice vectors were natural for integer indices, and so a large fraction of users will probably also have that thought. Of course there will be users that think the other way around, but regardless you'll get some users who don't expect the outcome of the arguments.

Does this solution make sense to you?

Ok, I see that we commented the same at the same time, then great!

yes, great. I think this is better. Also consistency in sisl and elsewhere is key, I use integers as lattice vector matches.

zerothi · 2021-08-12T20:17:33Z

Ok, I have amended another commit. Please ensure that you re-fetch your branch (I have overwritten it).

Sorry for doing it like this, but it cleared many warnings and added some things needed.

pfebrer · 2021-08-13T10:09:47Z

is this on purpose, or should this test actually have failed?

This warning is to be expected. These tests are just to check that the basic functionality of the Plot class has not been broken by the child class. The plots are initialized, but since they have no data to plot, they show this warning. I agree that this is annoying though when running the tests. Maybe we could filter the warnings in the test functions?

zerothi · 2021-08-13T10:45:21Z

is this on purpose, or should this test actually have failed?

This warning is to be expected. These tests are just to check that the basic functionality of the Plot class has not been broken by the child class. The plots are initialized, but since they have no data to plot, they show this warning. I agree that this is annoying though when running the tests. Maybe we could filter the warnings in the test functions?

Yes, I think we should filter, I tried to manage this in some of my tests, and this is still a work-in-progress. But it makes it much easier to figure out if something goes wrong. In fact pytest can check where a specific warning has been raised, see here

zerothi · 2021-08-13T11:58:45Z

I am done pushing to your branch, so perhaps you could have a go, and the we can merge this?

pfebrer · 2021-08-13T17:06:53Z

I removed all the warnings that are due to the failed initialization of the plot, now I only get some warnings for using MonkhorstPack.apply.average.PDOS():

  dep:0: SislDeprecation:
  
  use Hamiltonian.eigenstate(...).PDOS() instead [>=0.13.0]

I have two questions:

Why don't these warnings appear when I run the code in a notebook? This is the code that I run in the tests:

gr = sisl.geom.graphene()
H = sisl.Hamiltonian(gr)
H.construct([(0.1, 1.44), (0, -2.7)])

H.plot.pdos(Erange=[-5,5])

How should I go about calculating the PDOS using the monkhorst pack object if Hamiltonian.PDOS() is deprecated?

zerothi · 2021-08-13T17:54:19Z

I removed all the warnings that are due to the failed initialization of the plot, now I only get some warnings for using MonkhorstPack.apply.average.PDOS():
  dep:0: SislDeprecation:
  
  use Hamiltonian.eigenstate(...).PDOS() instead [>=0.13.0]
I have two questions:

Why don't these warnings appear when I run the code in a notebook? This is the code that I run in the tests:
gr = sisl.geom.graphene()
H = sisl.Hamiltonian(gr)
H.construct([(0.1, 1.44), (0, -2.7)])

H.plot.pdos(Erange=[-5,5])

Probably because the default level of the warnings module is too low.

How should I go about calculating the PDOS using the monkhorst pack object if Hamiltonian.PDOS() is deprecated?

One should pass a wrapper: mp.apply.average.eigenstate(wrap=lambda es: es.PDOS()), a bit clumsy, we should think of a way to incorporate class calls as deferred methods in dispatchers.

pfebrer · 2021-08-17T08:18:56Z

It took me a while to make geometries and grids play well with each other in all cases. This is what I ended up doing for the different possible values of axes:

{"x", "y", "z"}: The cartesian coordinates are displayed, for both GeometryPlot and GridPlot.
{"a", "b", "c"}: The fractional coordinates are displayed, for both GeometryPlot and GridPlot. Same for {0,1,2}.
np.array of shape (3, ): The coordinates are projected into that direction for GeometryPlot. If two directions are passed, the coordinates are not projected to each axis separately. The displayed coordinates are then the coefficients of the linear combination to get that point (or the projection of that point into the plane formed by the two axes). Not implemented in GridPlot.

zerothi · 2021-08-17T08:57:42Z

Sounds like a good compromise!

pfebrer · 2021-08-17T09:05:03Z

Good, then I just have to finish some details and update the documentation. Then we can merge this :)

pfebrer · 2021-08-17T18:22:08Z

I'd say it's finished now. There might still be some inconsistent things, but we can figure them out along the way, as we get some feedback :)

pfebrer · 2021-08-18T09:00:14Z

Do you plan to release a version soon? I would like to parallelize the PDOS and (fat)bands calculations (I will do it today) and agree on a CLI before the next release 😄

zerothi · 2021-08-18T09:03:01Z

No, we can wait for that. Definitely the cli would be great to have in the next release.

pfebrer · 2021-08-18T09:04:45Z

Good!

pfebrer marked this pull request as draft March 9, 2021 21:01

pfebrer force-pushed the detach-plotly branch from 5852865 to 953cf66 Compare March 19, 2021 09:09

pfebrer mentioned this pull request Mar 19, 2021

maint: Using dispatchers for plot handlers #317

Merged

pfebrer force-pushed the detach-plotly branch from 953cf66 to 5ea0b7a Compare March 31, 2021 20:07

pfebrer force-pushed the detach-plotly branch from 3f2b223 to ff2e117 Compare April 17, 2021 08:36

pfebrer mentioned this pull request May 28, 2021

Discussion on API to initialize MultiplePlot. #340

Open

zerothi reviewed Jun 1, 2021

View reviewed changes

pfebrer force-pushed the detach-plotly branch from ff2e117 to 6b33fb2 Compare June 1, 2021 16:47

pfebrer mentioned this pull request Jun 14, 2021

Brillouin zone xarray Dataset dispatch #349

Closed

zerothi reviewed Jun 15, 2021

View reviewed changes

pfebrer force-pushed the detach-plotly branch from 6b33fb2 to 65efdb6 Compare June 15, 2021 08:20

zerothi changed the title ~~WIP: Detaching plotly from the processing and allowing other drawers.~~ ENH: Detaching plotly from the processing and allowing other drawers. Jun 15, 2021

pfebrer marked this pull request as ready for review June 15, 2021 22:42

pfebrer added 5 commits August 12, 2021 21:35

all tests passing again

9094016

unskip tests

d36d7b8

wavefunction bug

0d0f4fa

fix session bugs

80df194

plotly is no longer a dependency of sisl.viz

759e6e4

zerothi force-pushed the detach-plotly branch from 5f9a2fa to 759e6e4 Compare August 12, 2021 19:35

zerothi reviewed Aug 12, 2021

View reviewed changes

enh: small edits, added ability to customize load

a05e7aa

mnt: avoid initialization warnings

c641768

pfebrer added 2 commits August 15, 2021 14:40

mnt: avoid PDOS warning

0d688f2

enh: rethink axes setting to be consistent within sisl

242b373

pfebrer mentioned this pull request Aug 17, 2021

mnt: changed all origo references to origin #365

Merged

2 tasks

maint: Added axes tests and documentation

13dad3d

zerothi merged commit 06b73a2 into zerothi:master Aug 17, 2021

pfebrer deleted the detach-plotly branch August 18, 2021 08:14

ENH: Detaching plotly from the processing and allowing other drawers. #312

ENH: Detaching plotly from the processing and allowing other drawers. #312

Conversation

pfebrer commented Mar 9, 2021 • edited Loading

PR introduction

What has been done

How it works

zerothi commented Mar 10, 2021

pfebrer commented Mar 10, 2021 • edited Loading

zerothi commented Mar 11, 2021

pfebrer commented Jun 1, 2021

zerothi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zerothi commented Jun 1, 2021

pfebrer commented Jun 1, 2021

pfebrer commented Jun 1, 2021

zerothi left a comment

Choose a reason for hiding this comment

pfebrer commented Jun 15, 2021

zerothi commented Jun 15, 2021

pfebrer commented Jun 15, 2021

pfebrer commented Jun 15, 2021

zerothi commented Jun 15, 2021

zerothi commented Jun 15, 2021

pfebrer commented Jun 15, 2021

zerothi commented Jun 16, 2021

zerothi commented Jun 16, 2021

pfebrer commented Jun 16, 2021 • edited Loading

zerothi commented Aug 12, 2021

zerothi commented Aug 12, 2021

Choose a reason for hiding this comment

pfebrer Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zerothi commented Aug 12, 2021

pfebrer commented Aug 13, 2021

zerothi commented Aug 13, 2021

zerothi commented Aug 13, 2021

pfebrer commented Aug 13, 2021

zerothi commented Aug 13, 2021

pfebrer commented Aug 17, 2021 • edited Loading

zerothi commented Aug 17, 2021

pfebrer commented Aug 17, 2021 • edited Loading

pfebrer commented Aug 17, 2021

pfebrer commented Aug 18, 2021 • edited Loading

zerothi commented Aug 18, 2021

pfebrer commented Aug 18, 2021

pfebrer commented Mar 9, 2021 •

edited

Loading

pfebrer commented Mar 10, 2021 •

edited

Loading

pfebrer commented Jun 16, 2021 •

edited

Loading

pfebrer Aug 13, 2021 •

edited

Loading

pfebrer commented Aug 17, 2021 •

edited

Loading

pfebrer commented Aug 17, 2021 •

edited

Loading

pfebrer commented Aug 18, 2021 •

edited

Loading