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

Epoch plots #2023

Open
wants to merge 69 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@amkearns-usgs
Copy link

amkearns-usgs commented Dec 6, 2017

What does this PR do?

This allows for inventory objects to have their epochs plotted as

Why was it initiated? Any relevant Issues?

See issue #1822

PR Checklist

  • Correct base branch selected? master for new fetures, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • +DOCS to see how documentation looks like
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .
  • add image test (can be done at the very end, when everybody is happy with how the plots look like; when adding test images, please do this in a separate commit to make squashing binary images easier at the end of the PR before merging)
  • add example plot to docstring of respective routine and link it in gallery page

@megies megies added this to the 1.2.0 milestone Dec 7, 2017

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 7, 2017

Mind posting an example image?

@amkearns-usgs

This comment has been minimized.

Copy link

amkearns-usgs commented Dec 7, 2017

figure_1

plt.show()


def _plot_traversal_helper_(plot_dict, y_dict, offset=0, prefix=''):

This comment has been minimized.

@megies

megies Dec 11, 2017

Member

no trailing underscore with method/function names, please, to be in line with rest of code base

This comment has been minimized.

@amkearns-usgs

amkearns-usgs Dec 11, 2017

I was doing that as a means of marking the functions that weren't intended to be callable by the user. Is the leading underscore still acceptable, or should I remove that as well?

This comment has been minimized.

@megies

megies Dec 11, 2017

Member

A single leading underscore is correct Python practice for private routines, just get rid of the trailing one please.

This comment has been minimized.

@dsentinel

dsentinel Dec 11, 2017

Contributor

Leading underscore denotes "private" function. Leave it.

return offset


def _plot_builder_(ax, plot_dict, y_dict, xmin, xmax, clrs, pfx=''):

This comment has been minimized.

@megies

megies Dec 11, 2017

Member

see above..

@@ -507,6 +508,34 @@ def plot(self, min_freq, output="VEL", location="*", channel="*",

return fig

def _get_epoch_plottable_struct_(self):

This comment has been minimized.

@megies

megies Dec 11, 2017

Member

see below.. no trailing underscore

@@ -675,6 +676,38 @@ def plot_response(self, min_freq, output="VEL", station="*", location="*",

return fig

def _get_epoch_plottable_struct_(self):

This comment has been minimized.

@megies

megies Dec 11, 2017

Member

see below.. no trailing underscore

@@ -892,6 +892,40 @@ def plot_response(self, min_freq, output="VEL", network="*", station="*",

return fig

def _get_epoch_plottable_struct_(self):

This comment has been minimized.

@megies

megies Dec 11, 2017

Member

see below.. no trailing underscore

@@ -365,6 +367,27 @@ def plot(self, min_freq, output="VEL", start_stage=None, end_stage=None,
unwrap_phase=unwrap_phase, plot_degrees=plot_degrees, show=show,
outfile=outfile)

def _get_epoch_plottable_struct_(self):

This comment has been minimized.

@megies

megies Dec 11, 2017

Member

see below.. no trailing underscore

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 11, 2017

This looks promising.. here's a few comments:

  • Looks like you're doing manual ticking on the x-Axis? This is in general not advisable as it might not update as expected when people zoom into plots etc etc. Can you have a look at how this is handled in obspy/imaging/waveform.py in WaveformPlotting.__plot_set_x_ticks() which is using helper routine _set_xaxis_obspy_dates(ax). Should be a one-liner to reuse that helper.
  • I'm not sure what units you are using on the x-Axis, but it looks like you might simply be converting UTCDateTime objects with float(..). If that's correct, it would mean you're using POSIX timestamps on the x-Axis. But ideally you should be using matplotlib's time-stamping conventions in plots, i.e. do something like (assuming my_time is a UTCDateTime instance): my_time.matplotlib_date. Obviously this is also needed for automatic x-ticking like mentioned above

I think it would be nice to have additional logical grouping with colors to guide the eyes..

  • you could use something like matplotlib.patches.Rectangle(fill=True, zorder=-5, alpha=0.2, facecolor=...) to give a slightly transparent background color to e.g. all stations from the same network
  • line widths should be increased I think, maybe to lw=2 or even lw=3
  • arbitrary color-cycling through each individual line does not bring a big advantage I think, maybe it might be an idea to use pre-defined color depending on sampling rate of channel? That way one could immediately see if a station has a high-sampling-rate channel available or not.

Let me know if above comments are unclear.

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 11, 2017

See circle CI log for things that need fixing for PEP8 compliance:

obspy/clients/httpproxy.py:115:9: E741 ambiguous variable name 'l'
obspy/clients/httpproxy.py:116:12: E741 ambiguous variable name 'l'
obspy/clients/earthworm/waveserver.py:242:13: E741 ambiguous variable name 'l'
obspy/clients/filesystem/tests/test_sds.py:222:17: E722 do not use bare except'
obspy/clients/seedlink/tests/test_basic_client.py:44:13: E722 do not use bare except'
obspy/core/trace.py:2415:9: E722 do not use bare except'
obspy/core/inventory/inventory.py:398:13: E722 do not use bare except'
obspy/core/inventory/inventory.py:910:23: F405 'UTCDateTime' may be undefined, or defined from star imports: future.builtins
obspy/core/inventory/inventory.py:912:42: F405 'UTCDateTime' may be undefined, or defined from star imports: future.builtins
obspy/core/inventory/network.py:688:30: E225 missing whitespace around operator
obspy/core/inventory/network.py:692:23: F405 'UTCDateTime' may be undefined, or defined from star imports: future.builtins
obspy/core/inventory/network.py:694:42: F405 'UTCDateTime' may be undefined, or defined from star imports: future.builtins
obspy/core/inventory/response.py:819:17: E722 do not use bare except'
obspy/core/util/testing.py:465:9: E722 do not use bare except'
obspy/imaging/cm.py:321:5: E741 ambiguous variable name 'l'
obspy/io/arclink/inventory.py:75:9: E722 do not use bare except'
obspy/io/arclink/inventory.py:89:9: E722 do not use bare except'
obspy/io/arclink/inventory.py:176:5: E722 do not use bare except'
obspy/io/arclink/inventory.py:192:5: E722 do not use bare except'
obspy/io/arclink/inventory.py:885:5: E722 do not use bare except'
obspy/io/mseed/scripts/recordanalyzer.py:145:9: E722 do not use bare except'
obspy/io/mseed/scripts/recordanalyzer.py:171:9: E722 do not use bare except'
obspy/io/mseed/scripts/recordanalyzer.py:228:13: E722 do not use bare except'
obspy/io/mseed/scripts/recordanalyzer.py:254:13: E722 do not use bare except'
obspy/io/mseed/scripts/recordanalyzer.py:265:13: E722 do not use bare except'
obspy/io/mseed/scripts/recordanalyzer.py:278:13: E722 do not use bare except'
obspy/io/xseed/utils.py:113:5: E741 ambiguous variable name 'l'
obspy/signal/freqattributes.py:61:28: E741 ambiguous variable name 'l'
obspy/signal/freqattributes.py:81:9: E741 ambiguous variable name 'l'
obspy/signal/freqattributes.py:82:9: E741 ambiguous variable name 'l'
obspy/signal/rotate.py:106:5: E741 ambiguous variable name 'l'
@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 11, 2017

Another thought.. instead of colors for sampling rate, we could also use linestyles to represent sampling rates.. e.g. dotted line for high sampling rates, dashed line for intermediate rates etc. (it's possible to customize line stroking pattern), that might be even more intuitive

@amkearns-usgs

This comment has been minimized.

Copy link

amkearns-usgs commented Dec 11, 2017

I've made most of the changes requested here so far, though I wouldn't be surprised if some of the specific parameters such as dash length will call for additional tuning.

How do we want to handle the question of coloring the lines and rectangles? While the plotter gives a new color to each line by default, all the rectangles have the same shade of blue to them.

@dsentinel

This comment has been minimized.

Copy link
Contributor

dsentinel commented Dec 11, 2017

I would avoid broken lines. May be interpreted as gaps. Even though this is from an inventory/metadata, people may carry over from time series coverage plots. Maybe, maybe it works.

@megies

This comment has been minimized.

Copy link
Member

megies commented Dec 12, 2017

@amkearns-usgs Can you post an updated image?

@amkearns-usgs

This comment has been minimized.

Copy link

amkearns-usgs commented Dec 12, 2017

Here's a cap of what it looks like now. I may still change the per-station colors to all match, so GR.WET.* all has the same color, for example.
figure_1

@amkearns-usgs

This comment has been minimized.

Copy link

amkearns-usgs commented Dec 12, 2017

I've gone ahead and enabled grouping colors by station, which should hopefully make it easier to distinguish between them. Networks should still be separated by a space like in this plot.

figure_1

Minor edit to make the boxes darker (it was difficult to distinguish the green lines over the original default blue color)

@@ -911,8 +912,8 @@ def _get_epoch_plottable_struct(self):
else:
end = min(self.end_time, UTCDateTime.now())

This comment has been minimized.

@megies

megies Dec 13, 2017

Member

I think we should plot what really is given for the endtime of Inventories etc. and not cut them off at current time. I'm assuming you did this to get the right end of the plot at current time? This can also be achieved by just setting the right x limit to current time at the very end of all plotting before popping up the plot window. But when people zoom out interactively, the actual end time can be seen.

For open ended epochs we could just leave away the end marker and have them end at current time

@megies

This comment has been minimized.

Copy link
Member

megies commented Feb 26, 2018

@amkearns-usgs these imports (that potentially set the matplotlib backend) should be done inside the routines that need that import. See e.g. https://github.com/amkearns-usgs/obspy/blob/fbb1ad7c04e1935d48e6fa4175fb6e745ddac7e4/obspy/imaging/waveform.py#L222-L230

@amkearns-usgs

This comment has been minimized.

Copy link

amkearns-usgs commented Feb 26, 2018

I have the test case set up and the image being compared against in the testing images subdirectory, but for some reason the test is failing on my end. Not sure why. (That said, I wouldn't be surprised to find that it was something simple like the generated plot shown in a window has a grey background, but the saved image has a white one.)

@megies

This comment has been minimized.

Copy link
Member

megies commented Feb 27, 2018

Uhm.. did you attempt a rebase? Hmm, or rather it looks like you just merged master branch? In any case, the history of your branch is utterly messed up, it really needs a clean up..

@megies

This comment has been minimized.

Copy link
Member

megies commented Feb 27, 2018

Regarding the test fails, there seems to be a problem with your baseline images: http://tests.obspy.org/92654/#1 and see https://raw.githubusercontent.com/amkearns-usgs/obspy/a0c2998d3c437e08a8d6466123db1ac51d16a104/obspy/core/tests/images/inventory-epoch-plot.png which also shows an error

@amkearns-usgs amkearns-usgs force-pushed the amkearns-usgs:epoch_plots branch from a0c2998 to 7e2c59e Mar 5, 2018

amkearns-usgs added some commits Mar 5, 2018

Delete inventory-epoch-plot.png
needs to be reuploaded as binary, causing issues with tests
@amkearns-usgs

This comment has been minimized.

Copy link

amkearns-usgs commented Mar 5, 2018

OK. I wasn't able to totally fix the history pollution but I have managed to get the png upload working properly and the test now passes.

@@ -864,7 +863,7 @@ def _seed_id_keyfunction(x):
return x


def plot_inventory_epochs(plot_dict, outfile=None, colorspace=None, show=True,
def plot_inventory_epochs(plot_dict, outfile=None, colorspace=None, show=False,

This comment has been minimized.

@megies

megies Mar 6, 2018

Member

I think usually we have show=True in all plots? I.e. having interactive plots pop up on default..

@megies

This comment has been minimized.

Copy link
Member

megies commented Apr 20, 2018

@amkearns-usgs I wanted to try this one with a real world inventory and ran into an exception..

from obspy.clients.fdsn import Client
client = Client()
inv = client.get_stations(network='IU')
inv.plot_epochs()

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/megies/git/obspy/obspy/core/inventory/inventory.py", line 953, in plot_epochs
    combine)
  File "/home/megies/git/obspy/obspy/core/inventory/util.py", line 896, in plot_inventory_epochs
    mg_dict = _combine_same_epochs(plot_dict)
  File "/home/megies/git/obspy/obspy/core/inventory/util.py", line 963, in _combine_same_epochs
    return _create_same_epochs_string(merge_dict, epochs_dict)
  File "/home/megies/git/obspy/obspy/core/inventory/util.py", line 1011, in _create_same_epochs_string
    loc = split[2]
IndexError: list index out of range

Also, when I do the plot with empty location codes, they show up in the axis labels as _ it seems..


One more thing.. it can be sometimes hard to see relatively short interruptions in channels, maybe it would be better to see if we changed the plot to one shaded box per epoch, rather than one box with overall start/end of channels?

@megies

This comment has been minimized.

Copy link
Member

megies commented May 7, 2018

This might yet take a bit of discussion and polishing, I think, so I will bump it to 1.3.0. Feel free to change back to 1.2.0 if you want to push this PR hard in the coming days @amkearns-usgs.

@megies megies modified the milestones: 1.2.0, 1.3.0 May 7, 2018

@amkearns-usgs

This comment has been minimized.

Copy link

amkearns-usgs commented May 7, 2018

Unfortunately I don't think I'll be able to do that as a more pressing deadline is coming up for other software I'm the main contributor for. I'm certainly interested in looking into this problem some more but I haven't been able to give it the time or attention needed recently on account of that.

@megies

This comment has been minimized.

Copy link
Member

megies commented May 8, 2018

Alright, no worries we can slate it for 1.3.0

@amkearns-usgs

This comment has been minimized.

Copy link

amkearns-usgs commented May 30, 2018

It's been a while since I've had time to look at this. Has the underlying structure for hardware epochs changed at all?

@amkearns-usgs

This comment has been minimized.

Copy link

amkearns-usgs commented May 31, 2018

Oh, here's a question related to a recent comment -- I wanted to use "_" to represent empty location codes to make it clear that an empty location field might exist in the case where we were plotting multiple channels' data on the same line. While it makes sense to use an empty string when only one channel is plotted to a line, it becomes ambiguous in the second case where some channels might have non-empty location codes, in which case having some symbol makes it easier to tell where locationless channels have been placed. If you have any recommendations on this I'd be interested to hear them.

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Jun 4, 2018

It's been a while since I've had time to look at this. Has the underlying structure for hardware epochs changed at all?

No.

Oh, here's a question related to a recent comment -- I wanted to use "_" to represent empty location codes to make it clear that an empty location field might exist in the case where we were plotting multiple channels' data on the same line. While it makes sense to use an empty string when only one channel is plotted to a line, it becomes ambiguous in the second case where some channels might have non-empty location codes, in which case having some symbol makes it easier to tell where locationless channels have been placed. If you have any recommendations on this I'd be interested to hear them.

I don't fully understand the question in the context of the plots to be honest. Can you clarify a bit?

The community in general seems to have arrived at the conclusion that an empty location code is still valid and _ would be different from an empty one. So I'd recommend to leave the empty code and in case multiple location codes are present in a single line, just use *?

@amkearns-usgs

This comment has been minimized.

Copy link

amkearns-usgs commented Jun 5, 2018

OK. What I've done over the previous implementation is have channels with matching epochs all get combined before they go into the plot. I've recently had to switch to a new laptop so my python installation requires me to do a few things before I can guarantee the tests are working the way I expect, but the bug mentioned earlier in this thread should hopefully have gone away. Also, I'm now listing merged channels like so: "NETWORK.STATION.[LOC1: [CH1, CH2], LOC2: [CH3, CH4]]". This representation is less compact but also less subject to ambiguities (i.e., it's clearer when two channels from different locations are merged together, especially if one has an empty string for location field).

I'll update the image used in the test case as soon as I solve my python issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment