Refactoring of code: separation of concerns for ploting variograms

[1kastner]

I want to display an experimental variogram even before deciding which kriging method I want to use. Now what I can do is using pykrige.ok.OrdinaryKriging.display_variogram_model or pykrige.uk.UniversalKriging.display_variogram_model. Two issues come to my mind:

• The two methods have some code duplicaiton, it could be solved with some programming pattern like delegation or inheritance
• It forces me to assume a variogram model which will be plotted as well. But I only want the experimental variogram.

My idea is to extract a visualizer as a class on its own that can does the plotting. That visualizer is used by any call of display_variogram_model and keeps the matplotlib code at one place. If somebody does not have matplotlib installed, it only needs to be handled in the visualizer class. Further it can give the freedom to plot only the experimental variogram as well.

[rth]

+1 for making the visualization more modular and for making matplotlib an optional dependency #63. Not sure if a separate class or function for plotting would be more appropriate (we should talk about the API in more detail). We might also consider deprecating (then removing after a while) the enable_plotting parameter and plotting only when display_variogram_model is explicitly called?

Generally, this is very much in line with refactoring proposed in #31 though addressing plotting instead of ND array handling...

What do you think?

[1kastner]

On first glance a function seems sufficient if we just want to do a simple refactoring but a class could offer more freedom for adjustments, e.g. allowing the end user to add x-axis or y-axis titles, change the default colors etc. - things which might be of importance for publications.

I agree that enable_plotting should be deprecated as you suggested.

[rth]

a class could offer more freedom for adjustments, e.g. allowing the end user to add x-axis or y-axis titles, change the default colors etc.

Well if the function returns matplotlib.AxesSubplot (ax), users could then style the resulting plot as needed (same as it is done e.g. in pandas.DataFrame.plot()). Also cf. second point from here.

I don't use the plotting functionality myself, so I don't have a very good understanding of all the use-cases (and on whether it should be a class or a function). @bsmurphy or @basaks might have a better idea..

[1kastner]

Well, it is possible but stepping through all matplotlib elements can turn out quite nasty because you need some in-depth understanding of how to access and modify the elements. So for me pandas.DataFrame.plot() woke up some bad memories...

[rth]

I know matplotlib API can be not very straightforward, but still IMO,

ax = plot_variogram(... )
ax.set_xlabel("some label")

that would also support defining the figure externally,

fix, ax = plt.subplots(111)
ax = plot_variogram(..., ax=ax)
ax.set_xlabel("some label")

is preferable to

plot_variogram(... , xlabel="some label")

both because it means less maintenance (and potential issues) in PyKrige and because one can never know what the user might want to do with the plot (e.g. semi-log scales, some color styles etc) and we can't add all the posible matplotlib options as input arguments...

[1kastner]

The last negative example is a total no-go, my idea is to use a delegation pattern so that the user can plug in their personal plotting function if they want. The visualisation class can have methods like plot_experimental_semivariogram or plot_semivariogram_model, invoke a before- and an after-method which can be filled with whatever the user needs. The last invocation would be plt.show().

Here as a sketch:

class Visualiser:
    def after_plot_experimental_semivariogram(self, options):
        pass
    def initialise_plot_experimental_semivariogram(self, options):
        fig = plt.figure()
        ax = fig.add_subplot(111)
        return axes 
    def plot_experimental_semivariogram(self, options):
        self.initialise_plot_experimental_semivariogram(options)
        # do the plotting
        self.after_plot_experimental_semivariogram(axes, options)
        plt.show()

This way the user can adjust plotting easily:

class MyVisualiser(Visualiser):
    def after_plot_experimental_semivariogram(self, options):
        plt.xlabel("heyho")

initialise_plot_experimental_semivariogram is important to control where to plot something.
after_plot_experimental_semivariogram is important to modify the chosen ticks, introduce a grid etc.

[rth]

@1kastner Personally, I'm less in favour of creating custom classes for the public API in general, as opposed to some minimalistic functions as it takes more cognitive effort to understand how it works: i.e. in this case, instead of just applying what they know about matplotlib (that has very extensive documentation), users will have to understand how this custom mechanism works (and our documentation is not so great at the moment), which methods need to be subclassed etc. There is also a number of corner cases that's I'm not sure we really want to deal with: e.g. what if I make a plot in a jupyter notebook and don't need to call plt.show(), what if I have some large dataset and I want to subsample or do parallel processing of it before variogram visualization, etc. Also once it is added to the public API, it will need to be maintained for the foreseeable future (e.g. can't just rename initialise_plot_experimental_semivariogram method if we find a better name for it, as it will break users code). So the simpler it is, the simpler it would be to maintain and use, and in particular, I can't think of any widely used Python package that would require subclassing to make a plot..

But anyway, you are very welcome to make a Pull Request, other people might have other perspectives on this as well. )

[1kastner]

1. Yes, the proposed pattern is not used commonly in Python, at least not the small github projects available to public 2. When you check the pattern - subclassing is not necessarily needed, that would be just for convenience. It is not a must. 3. If you feel more comfortable, a function could do the work as well. That could be some drawing function which can be replaced by the user

[rth]

Makes sense. In any case, you are welcome to submit a PR to make the visualization better )

…

On 23/08/17 09:14, 1kastner wrote: 1. Yes, the proposed pattern is not used commonly in Python, at least not the small github projects available to public 2. When you check the pattern - subclassing is not necessarily needed, that would be just for convenience. It is not a must. 3. If you feel more comfortable, a function could do the work as well. That could be some drawing function which can be replaced by the user Am 22. August 2017 15:54:20 MESZ schrieb Roman Yurchak ***@***.***>: ***@***.*** Personally, I'm less in favour of creating custom classes for >the public API in general, as opposed to some minimalistic functions as >it takes more cognitive effort to understand how it works: i.e. in this >case, instead of just applying what they know about matplotlib (that >has very extensive documentation), they will have to understand how >this custom mechanism works (and our documentation is not so great at >the moment), which methods need to be subclassed etc. There is also a >number of corner cases that's I'm not sure we really want to deal with: >e.g. what if I make a plot in a jupyter notebook and don't need to call >`plt.show()`, what if I have some large dataset and I want to subsample >or do parallel processing of it before variogram visualization, etc. >Also once it is added to the public API, it will need to be maintained >for the foreseeable future (e.g. can't just rename >`initialise_plot_experimental_semivariogram` method if we find a better >name for it, as it will break users code). So the simpler it is, the >simpler it would be to maintain and use, and in particular, I can't >think of any widely used Python package that would require subclassing >to make a plot.. > >But anyway, you are very welcome to make a Pull Request, other people >might have other perspectives on this as well. ) > >-- >You are receiving this because you were mentioned. >Reply to this email directly or view it on GitHub: >#72 (comment) -- Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#72 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAmgmH7G9JKmU737DcC8c_n5sAgbzHymks5sa8NhgaJpZM4O7PJy>.

[MuellerSeb]

Since we are aiming to use the covariance/variogram models of GSTools, it will be possible to use the plotting routines of GSTools in the future.
Discussion can continue here: #136
Closing for now. Feel free to re-open or (better) discuss in the linked issue.