eqcorrscan / EQcorrscan

Earthquake detection and analysis in Python.
https://eqcorrscan.readthedocs.io/en/latest/
Other
166 stars 86 forks source link

Add plotdir throughout #330

Closed calum-chamberlain closed 5 years ago

calum-chamberlain commented 5 years ago

What does this PR do?

Adds a standardised plotdir argument to:

This also adds some of the minor changes in #329.

Why was it initiated? Any relevant Issues?

329

PR Checklist

ikahbasi commented 5 years ago

it looks very good. isn't it better write os.makedirs in _finalise_figure() for one time and remove it from all other functions? (i wrote it please see) And instead of make & update "plot_kwargs" update main "kwargs"? in this way user can define kwargs and update inside code, both of user kwargs and plot_kwargs will be use, and kwargs can pass from function to another inside function.

calum-chamberlain commented 5 years ago

So the way you have it seems confusing to me:

  1. If you pass a savefile to either of these two plots it gets used as a directory name and the actual filename is changed. This is different to every other plotting function there, so seems like a bad idea
  2. In this way the user has no control over the actual name of the plot.
  3. If a user passes savefile in the normal way (like "directory/filename.format") then a directory with a .format is created. This is strange
  4. You are passing the plotdir kwarg to the plotting functions, but this isn't actually used by the plotting functions as far as I can tell.

The way I would like it to work is:

  1. Plotting API is left relatively as-is, plots should have a similar interface to one another (which I am generally bad at). The plotting API should be seen as something to be used by a user in scripts;
  2. Any additional things required by other functions in EQcorrscan should handle those additional things, whether this be by making directories, or post/pre processing figures.
ikahbasi commented 5 years ago

Hi, dear Chamberlain. answer to 3) with this method in _finalise_figure()

    if save:
        if '/' in savefile:
            plotdir = '/'.join(savefile.split('/')[:-1])
            if not os.path.isdir(plotdir):
                os.makedirs(plotdir)

savefile split with '/' and result for "directory/filename.format" is not ".format". it is only "directory".

but let's move on. Please tell me how can i edit #329 like things you would like?

calum-chamberlain commented 5 years ago

While that is true for all other functions, this section:

    plotname = '{}_SNR.png'.format(signal[0].stats.starttime)
    if "savefile" in kwargs.keys():
        savefile = "{}/{}".format(kwargs.get("savefile"), plotname)

takes a users savefile and adds an additional filename on the end, so the savefile passed to _finalise_figure is not the same as passed in.

RE what to do about #329: As I said, if this PR gets the result you were after (being able to pass a plotdir argument to key functions and making some small changes to figure format) then I would close #329 and I will merge this PR later today.