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

Decouple from matplotlib and add Bokeh for backend #57

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
95 changes: 40 additions & 55 deletions src/grid_strategy/_abc.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
import matplotlib.pyplot as plt
import numpy as np
Copy link
Member

Choose a reason for hiding this comment

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

LGTM bot thing is complaining that these imports are only necessary for the matplotlib backend.


from .backends.mtpltlib import Matplotlib
from .backends.bkh import Bokeh


class GridStrategy(metaclass=ABCMeta):
"""
Expand All @@ -15,8 +18,38 @@ class GridStrategy(metaclass=ABCMeta):
nearly square (nearly equal in both dimensions).
"""

def __init__(self, alignment="center"):
def __init__(self, alignment="center", backend="matplotlib"):
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of taking a string that says what the backend is, we should define an abstract base class and/or a Protocol and take that.

This will allow anyone to write their own backends for it without needing to submit them upstream.

Copy link
Member

Choose a reason for hiding this comment

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

Also, I notice that the backend objects take "alignment" and so does GridStrategy, which seems wrong, no?

I think if anything the alignment should be passed to the relevant backend objects' functions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was under the impression that Python didn't have this kind of static typing stuff. This is pretty cool. I'll look into it if I have time.

Copy link
Member

Choose a reason for hiding this comment

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

You don't have to worry too much about the protocol stuff. Make it an ABC to start with to organizer your thinking - the point is to advertise exactly what parts of the interface are public.

self.alignment = alignment
self.supported_backends = ["matplotlib", "bokeh"]
self.library = None

assert (
backend in self.supported_backends
), f"Library {backend} is not a supported backend."
if backend == "matplotlib":
try:
import matplotlib
except ImportError:
print(
"matplotlib not installed. Please install it to use it with grid_strategy."
)
self.library = Matplotlib(alignment=self.alignment)

elif backend == "bokeh":
try:
import bokeh
except ImportError:
print(
Copy link
Member

Choose a reason for hiding this comment

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

You should raise your own ImportError here, though actually I'm thinking that we can move this stuff out of the constructor entirely.

Considering my other suggestion, to take a GridStrategyBackend or whatever class (I spent literally no time thinking of that name) instead of a string, I think you can do this at module level:

DEFAULT_BACKEND = None
try:
    from .matplotlib_backend import MatplotlibBackend
    DEFAULT_BACKEND = MatplotlibBackend
except ImportError:
    pass

if DEFAULT_BACKEND is None:
    try:
        from .bokeh_backend import BokehBackend
        DEFAULT_BACKEND = BokehBackend
    except ImportError:
        pass

if DEFAULT_BACKEND is None:
    raise ImportError("Could not find a suitable backend - please install either matplotlib or bokeh")

Then have the constructor take backend=DEFAULT_BACKEND.

In the future we can rework this with importlib and a looping over a list of supported backends, but I figured this is a reasonable start when there's just the two.

"Bokeh is not installed. Please install it to use it with grid_strategy."
)
self.library = Bokeh(alignment=self.alignment)

# elif backend == "plotly":
Copy link
Member

Choose a reason for hiding this comment

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

I generally recommend against checking in commented-out code, we can move this to another branch or a later commit, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah, i didnt want to get rid of it so I didn't forget how I implemented it before hand. It had a bug that was difficult to understand so I just dropped it to implement bokeh instead.

# try:
# import plotly
# except ImportError:
# print("plotly not installed. Please install it to use it with grid_strategy.")
# self.library = Plotly(alignment=self.alignment)

def get_grid(self, n):
"""
Expand All @@ -30,70 +63,22 @@ def get_grid(self, n):
where each x would be a subplot.
"""

if n < 0:
raise ValueError
grid_arrangement = self.get_grid_arrangement(n)
return self.get_gridspec(grid_arrangement)
return self.get_figures(grid_arrangement)

@classmethod
@abstractmethod
def get_grid_arrangement(cls, n): # pragma: nocover
pass

def get_gridspec(self, grid_arrangement):
def get_figures(self, grid_arrangement):
nrows = len(grid_arrangement)
ncols = max(grid_arrangement)

# If it has justified alignment, will not be the same as the other alignments
if self.alignment == "justified":
return self._justified(nrows, grid_arrangement)
else:
return self._ragged(nrows, ncols, grid_arrangement)

def _justified(self, nrows, grid_arrangement):
ax_specs = []
num_small_cols = np.lcm.reduce(grid_arrangement)
gs = gridspec.GridSpec(
nrows, num_small_cols, figure=plt.figure(constrained_layout=True)
)
for r, row_cols in enumerate(grid_arrangement):
skip = num_small_cols // row_cols
for col in range(row_cols):
s = col * skip
e = s + skip

ax_specs.append(gs[r, s:e])
return ax_specs

def _ragged(self, nrows, ncols, grid_arrangement):
if len(set(grid_arrangement)) > 1:
col_width = 2
return self.library._justified(nrows, grid_arrangement)
else:
col_width = 1

gs = gridspec.GridSpec(
nrows, ncols * col_width, figure=plt.figure(constrained_layout=True)
)

ax_specs = []
for r, row_cols in enumerate(grid_arrangement):
# This is the number of missing columns in this row. If some rows
# are a different width than others, the column width is 2 so every
# column skipped at the beginning is also a missing slot at the end.
if self.alignment == "left":
# This is left-justified (or possibly full justification)
# so no need to skip anything
skip = 0
elif self.alignment == "right":
# Skip two slots for every missing plot - right justified.
skip = (ncols - row_cols) * 2
else:
# Defaults to centered, as that is the default value for the class.
# Skip one for each missing column - centered
skip = ncols - row_cols

for col in range(row_cols):
s = skip + col * col_width
e = s + col_width

ax_specs.append(gs[r, s:e])

return ax_specs
return self.library._ragged(nrows, ncols, grid_arrangement)
Empty file.
81 changes: 81 additions & 0 deletions src/grid_strategy/backends/bkh.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
from bokeh.layouts import layout, Spacer
from bokeh.io import show, output_file


class JustifiedGrid:
Copy link
Member

Choose a reason for hiding this comment

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

These two seem to share a decent amount of code, and I'm assuming they entirely share their substantive interface - maybe refactor into a BokehGrid abstract base class, possibly implementing add_plots, output_file, etc.

def __init__(self, nrows, grid_arrangement):
self.plots = [[]]
self.grid_arrangement = grid_arrangement
self.current_row = 0
self.nrows = nrows

def add_plot(self, plot):
self.plots[self.current_row].append(plot)
if len(self.plots[self.current_row]) == self.grid_arrangement[self.current_row]:
self.plots.append([])
self.current_row += 1
assert (
Copy link
Member

Choose a reason for hiding this comment

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

Just to clarify - you should not use assertions for exception handling. When to use assertions is a tricky business, but basically think that assertion code won't be run at all in some circumstances (if you run python -O, for example), so assertions are something where it will help you catch a bug if you're debugging the thing and it might fail in a mysterious way later, but not to be used to convey any information to the user, like, "This critical thing failed."

Not quite sure what category this fits in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll switch it to a try except block I think, since I think it would be fair to consider that user's requesting for n grids and then trying to insert more than n figures would be considered an error.

Copy link
Member

Choose a reason for hiding this comment

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

  1. I think you are right, and in fact I think you should move this to the top of add_plot, because you mutate the state of the backend in this function before you check whether it's valid to do so.

  2. To clarify, it's not try/except block, it's raise, probably something like raise RuntimeError(...), but you can look through the Exception hierarchy to see if anything suits your taste.

self.current_row <= self.nrows
), "Error: More graphs added to layout than previously specified."

def add_plots(self, plot_list):
for plot in plot_list:
self.add_plot(plot)

def output_dest(self, file):
output_file(file)
Copy link
Member

Choose a reason for hiding this comment

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

I don't know bokeh - were you supposed to return this, or is it the side effect you care about?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

bokeh creates html files as it's output medium, so that function doesn't return anything useful. I just sets the output file.


def show_plot(self):
l = layout(self.plots, sizing_mode="stretch_width")
show(l)


class AlignedGrid:
def __init__(self, nrows, ncols, grid_arrangement, alignment):
self.plots = [[]]
self.grid_arrangement = grid_arrangement
self.alignment = alignment
self.current_row = 0
self.nrows = nrows

def add_plot(self, plot):
self.plots[self.current_row].append(plot)
if len(self.plots[self.current_row]) == self.grid_arrangement[self.current_row]:
self.plots.append([])
self.current_row += 1
assert (
self.current_row <= self.nrows
), "Error: More graphs added to the layout than previously specified."

def add_plots(self, plots):
for plot in plots:
self.add_plot(plot)

def output_dest(self, file):
output_file(file)

def show_plot(self):
for row in self.plots:
if len(row) == max(self.grid_arrangement):
continue
else:
if self.alignment == "left":
row.append(Spacer())
elif self.alignment == "right":
row.insert(0, Spacer())
elif self.alignment == "center":
row.append(Spacer())
row.insert(0, Spacer())
l = layout(self.plots, sizing_mode="scale_both")
show(l)


class Bokeh:
def __init__(self, alignment):
self.alignment = alignment

def _justified(self, nrows, grid_arrangement):
return JustifiedGrid(nrows, grid_arrangement)

def _ragged(self, nrows, ncols, grid_arrangement):
return AlignedGrid(nrows, ncols, grid_arrangement, self.alignment)
90 changes: 90 additions & 0 deletions src/grid_strategy/backends/mtpltlib.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
from matplotlib import gridspec
import matplotlib.pyplot as plt
import numpy as np


class Matplotlib:
def __init__(self, alignment="center"):
Copy link
Member

Choose a reason for hiding this comment

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

Probably something for a future change, but I do think it would be nice to be able to pass a pre-existing figure to be populated by the backend.

I think I didn't see a great way to do that without doubling down on the tight coupling to matplotlib, but if you have users pass their own backends to the the GridStrategy objects, this allows you to configure the backend objects however you want, including passing them figures and whatnot.

self.alignment = alignment

def _justified(self, nrows, grid_arrangement):
ax_specs = []
num_small_cols = np.lcm.reduce(grid_arrangement)
gs = gridspec.GridSpec(
nrows, num_small_cols, figure=plt.figure(constrained_layout=True)
)
for r, row_cols in enumerate(grid_arrangement):
skip = num_small_cols // row_cols
for col in range(row_cols):
s = col * skip
e = s + skip

ax_specs.append(gs[r, s:e])
return ax_specs

def _ragged(self, nrows, ncols, grid_arrangement):
if len(set(grid_arrangement)) > 1:
col_width = 2
else:
col_width = 1

gs = gridspec.GridSpec(
nrows, ncols * col_width, figure=plt.figure(constrained_layout=True)
)

ax_specs = []
for r, row_cols in enumerate(grid_arrangement):
# This is the number of missing columns in this row. If some rows
# are a different width than others, the column width is 2 so every
# column skipped at the beginning is also a missing slot at the end.
if self.alignment == "left":
# This is left-justified (or possibly full justification)
# so no need to skip anything
skip = 0
elif self.alignment == "right":
# Skip two slots for every missing plot - right justified.
skip = (ncols - row_cols) * 2
else:
# Defaults to centered, as that is the default value for the class.
# Skip one for each missing column - centered
skip = ncols - row_cols

for col in range(row_cols):
s = skip + col * col_width
e = s + col_width

ax_specs.append(gs[r, s:e])

return ax_specs


# class Plotly:
Copy link
Member

Choose a reason for hiding this comment

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

Why is this all commented out? Does it not work yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above, I will work on it later.

# from plotly.subplots import make_subplots
# import numpy as np

# def __init__(self, alignment="center"):
# self.alignment = alignment

# def _justified(self, nrows, grid_arrangement):
# num_small_cols = int(self.np.lcm.reduce(grid_arrangement))

# specs = []

# for row_cols in grid_arrangement:
# width = num_small_cols // row_cols

# row = []
# for _ in range(row_cols):
# row.append({"colspan":width})
# row.extend([None]*(width-1))
# specs.append(row)

# fig = self.make_subplots(
# nrows,
# num_small_cols,
# specs = specs
# )
# return fig, grid_arrangement

# def _ragged(self, nrows, ncols, grid_arrangement):
# pass
1 change: 1 addition & 0 deletions tests/test_grids.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from unittest import mock

from grid_strategy.strategies import SquareStrategy
import grid_strategy.backends


class SpecValue:
Expand Down
4 changes: 3 additions & 1 deletion tox.ini
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
[tox]
envlist = py36, py37, black-check
envlist = py36, py37, py38, black-check
skip_missing_interpreters = true
isolated_build = true

Expand All @@ -10,6 +10,7 @@ deps =
pytest
pytest-cov >= 2.0.0
coverage
bokeh
commands = pytest --cov-config="{toxinidir}/tox.ini" \
--cov="{envsitepackagesdir}/grid_strategy" \
--cov="{toxinidir}/tests" \
Expand Down Expand Up @@ -40,6 +41,7 @@ show_missing = True
description = test if black works
deps =
pytest-black
bokeh
commands = pytest --black

[testenv:docs]
Expand Down