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

ray path plotting routine #1236

Closed
wants to merge 76 commits into
base: master
from

Conversation

@MMesch
Contributor

MMesch commented Feb 5, 2016

This is probably another longer duration pull request. It adds functions for greatcircle path plotting in 2D and 3D to obspy, based on geographiclib and taup. It is already working but needs to be structured nicely. Here is an example, plotted again with mayavi (green = P, blue = Pdiff, red = PKP), but plotting could be done with matplotlib and vtk file output again. Right now it works for one event and and inventory, but I think now it is easy to expand it to a catalog and an inventory together.

3d_paths

+DOCS

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Feb 6, 2016

Member

Perhaps you have missed it (likely because of our slow release rate), but there is already a plotting routine in TauPyModel (though not in 3D) as well as a function to get ray paths via geographic coordinates.

Expanding the existing stuff to 3D and to handle an entire Catalog/Inventory would certainly be interesting.

Member

QuLogic commented Feb 6, 2016

Perhaps you have missed it (likely because of our slow release rate), but there is already a plotting routine in TauPyModel (though not in 3D) as well as a function to get ray paths via geographic coordinates.

Expanding the existing stuff to 3D and to handle an entire Catalog/Inventory would certainly be interesting.

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 6, 2016

Contributor

ok I use the taup function now and put the subroutines directly into the inventory class. I'll add some non-mayavi plotting options later...

3d_paths

Contributor

MMesch commented Feb 6, 2016

ok I use the taup function now and put the subroutines directly into the inventory class. I'll add some non-mayavi plotting options later...

3d_paths

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Feb 6, 2016

Member

Looks great :-)

In terms of code structure I think it would be much better to but all plotting code into obspy.imaging and just add thin wrappers to the inventory class. If we start to add fancy plotting to all core objects it will really bloat the core module. The same might be good for the moment tensor plotting things in the other PR.

class Inventory(...):
    ...

    def plot_rays(self, *args, **kwargs):
        from obspy.imaging.inventory import plot_rays
        plot_rays(inventory=self, *args, **kwargs)

The plot_rays() function should probably also take event objects. The optional event coordinates are just there for convenience.

plot_rays(inventory, event=None, event_latitude=None,
          event_longitude=None, event_depth_in_m=None):
    ...
Member

krischer commented Feb 6, 2016

Looks great :-)

In terms of code structure I think it would be much better to but all plotting code into obspy.imaging and just add thin wrappers to the inventory class. If we start to add fancy plotting to all core objects it will really bloat the core module. The same might be good for the moment tensor plotting things in the other PR.

class Inventory(...):
    ...

    def plot_rays(self, *args, **kwargs):
        from obspy.imaging.inventory import plot_rays
        plot_rays(inventory=self, *args, **kwargs)

The plot_rays() function should probably also take event objects. The optional event coordinates are just there for convenience.

plot_rays(inventory, event=None, event_latitude=None,
          event_longitude=None, event_depth_in_m=None):
    ...
@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 6, 2016

Contributor

OK agree. I just don't see that these things are really 'imaging' in the sense of tomography. They are more visualization routines. That's why it seems strange to put them in this module.

Contributor

MMesch commented Feb 6, 2016

OK agree. I just don't see that these things are really 'imaging' in the sense of tomography. They are more visualization routines. That's why it seems strange to put them in this module.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Feb 6, 2016

Member

Haha...i did not realize that confusion. obspy.imaging actually is the visualization module of ObsPy ;-) I'm so used to the name I never saw that the name is ambiguous in seismology...

Member

krischer commented Feb 6, 2016

Haha...i did not realize that confusion. obspy.imaging actually is the visualization module of ObsPy ;-) I'm so used to the name I never saw that the name is ambiguous in seismology...

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 6, 2016

Contributor

ok I'll try to see it as visualization then :)

Contributor

MMesch commented Feb 6, 2016

ok I'll try to see it as visualization then :)

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 6, 2016

Contributor

looks pretty cool now if I may say so :)

P
3d_paths

PKiKP
pkikp

PPP
ppp

Pdiff
pdiff

PcP
pcp

PKiKP bright colorscheme
pkikp_bright

Contributor

MMesch commented Feb 6, 2016

looks pretty cool now if I may say so :)

P
3d_paths

PKiKP
pkikp

PPP
ppp

Pdiff
pdiff

PcP
pcp

PKiKP bright colorscheme
pkikp_bright

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Feb 7, 2016

Member

Bright color scheme is much better to see for me, at least in the examples above..

Member

megies commented Feb 7, 2016

Bright color scheme is much better to see for me, at least in the examples above..

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Feb 8, 2016

Member

It looks like you're creating your own colour scheme by hand. That should not be necessary. Since we have a hard dependency on matplotlib, you can just pull the colour map from there. Even better would be to use the existing colour set.

Also, it seems you have not configured your git client with your email address, is there a reason you have not done so? The first commit here is attributed to <matthias@matthias-Latitude-E6520.(none)> and the rest to <MMesch@users.noreply.github.com>. It is okay if you do not wish to expose your email address, but it would be best if a consistent address were used.

Member

QuLogic commented Feb 8, 2016

It looks like you're creating your own colour scheme by hand. That should not be necessary. Since we have a hard dependency on matplotlib, you can just pull the colour map from there. Even better would be to use the existing colour set.

Also, it seems you have not configured your git client with your email address, is there a reason you have not done so? The first commit here is attributed to <matthias@matthias-Latitude-E6520.(none)> and the rest to <MMesch@users.noreply.github.com>. It is okay if you do not wish to expose your email address, but it would be best if a consistent address were used.

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 8, 2016

Contributor

ok. I use the taup colors now but I have to change the lightness and saturation because it is essential to see something in the 3d plots.

sorry about my email address, I'll try not to change it anymore!

Contributor

MMesch commented Feb 8, 2016

ok. I use the taup colors now but I have to change the lightness and saturation because it is essential to see something in the 3d plots.

sorry about my email address, I'll try not to change it anymore!

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Feb 8, 2016

Member

I pushed some small things to make it a bit easier to use. Looks really cool :-)

A couple notes:

  • Diffracted phases need to be interpolated as they only store two points along the whole diffracted part of the phase which results in a straight line in xyz which is wrong. This probably has to go into the geographic ray calculation routine.
  • The colors don't seem to work for me - I'll look into that.
  • Mayavi as a whole seems very unstable - it keeps crashing randomly for me.
Member

krischer commented Feb 8, 2016

I pushed some small things to make it a bit easier to use. Looks really cool :-)

A couple notes:

  • Diffracted phases need to be interpolated as they only store two points along the whole diffracted part of the phase which results in a straight line in xyz which is wrong. This probably has to go into the geographic ray calculation routine.
  • The colors don't seem to work for me - I'll look into that.
  • Mayavi as a whole seems very unstable - it keeps crashing randomly for me.
@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Feb 9, 2016

Member

Considering that Mayavi is very unstable for me I played around a bit and maybe we can do this completely in WebGL. The nice thing is that the only dependency is a web browser which everyone has and it would even work in the notebooks. Also the render quality seems to be higher (I cannot get mayavi to render with antialiasing) and it is probably a bit easier to add interactivity.

Super unpolished toy example (12 MB of data...so you might have to wait a second): http://www.geophysik.uni-muenchen.de/~krischer/raypaths/

Member

krischer commented Feb 9, 2016

Considering that Mayavi is very unstable for me I played around a bit and maybe we can do this completely in WebGL. The nice thing is that the only dependency is a web browser which everyone has and it would even work in the notebooks. Also the render quality seems to be higher (I cannot get mayavi to render with antialiasing) and it is probably a bit easier to add interactivity.

Super unpolished toy example (12 MB of data...so you might have to wait a second): http://www.geophysik.uni-muenchen.de/~krischer/raypaths/

@claudiodsf

This comment has been minimized.

Show comment
Hide comment
@claudiodsf

claudiodsf Feb 9, 2016

Member

@krischer Wow! That is impressive! (except for the earthquake at north pole :). Which tool do you use to generate WebGL?

Member

claudiodsf commented Feb 9, 2016

@krischer Wow! That is impressive! (except for the earthquake at north pole :). Which tool do you use to generate WebGL?

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Feb 9, 2016

Member

Yea...the coordinate system for the rays (or the textures) has to be rotated so its not fully correct as of now ;-)

This uses three.js: http://threejs.org/

Member

krischer commented Feb 9, 2016

Yea...the coordinate system for the rays (or the textures) has to be rotated so its not fully correct as of now ;-)

This uses three.js: http://threejs.org/

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch
Contributor

MMesch commented Feb 9, 2016

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 9, 2016

Contributor

@krischer great thing!!! This will certainly attract some people to the lmu server :)

Contributor

MMesch commented Feb 9, 2016

@krischer great thing!!! This will certainly attract some people to the lmu server :)

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 9, 2016

Contributor

also hopefully to obspy

Contributor

MMesch commented Feb 9, 2016

also hopefully to obspy

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 9, 2016

Contributor

my mayavi is absolutely stable btw. Maybe it is an issue of your graphic cards driver?

Contributor

MMesch commented Feb 9, 2016

my mayavi is absolutely stable btw. Maybe it is an issue of your graphic cards driver?

@ThomasLecocq

This comment has been minimized.

Show comment
Hide comment
@ThomasLecocq

ThomasLecocq Feb 9, 2016

Contributor

@MMesch WOOOOW amazing!

Contributor

ThomasLecocq commented Feb 9, 2016

@MMesch WOOOOW amazing!

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 9, 2016

Contributor

Another though (sorry about all the mini posts):

The Pdiff issue should be solved in the taup module directly in my opinion

Contributor

MMesch commented Feb 9, 2016

Another though (sorry about all the mini posts):

The Pdiff issue should be solved in the taup module directly in my opinion

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Feb 9, 2016

Member

Great movie :-)

@krischer great thing!!! This will certainly attract some people to the lmu server :)

I'll give this a real shot then. I'll try to mimik your plot and the website will just be output of inv.plot_rays(). A really cool side effect is that it works on Python 3, has zero dependencies, and could also be embedded in the notebooks.

my mayavi is absolutely stable btw. Maybe it is an issue of your graphic cards driver?

Hmm...who knows :-( I have a macbook with the same graphic card as ten million other people and it usually works fine enough.

The Pdiff issue should be solved in the taup module directly in my opinion

I'm not sure - it is not wrong in the spherical coordinate systems. The plotting just forces a cartesian interpolation which is wrong. But it has to happen before the geographical routines take over, otherwise we have to deal with circular longitudes and wrap around effects...

Member

krischer commented Feb 9, 2016

Great movie :-)

@krischer great thing!!! This will certainly attract some people to the lmu server :)

I'll give this a real shot then. I'll try to mimik your plot and the website will just be output of inv.plot_rays(). A really cool side effect is that it works on Python 3, has zero dependencies, and could also be embedded in the notebooks.

my mayavi is absolutely stable btw. Maybe it is an issue of your graphic cards driver?

Hmm...who knows :-( I have a macbook with the same graphic card as ten million other people and it usually works fine enough.

The Pdiff issue should be solved in the taup module directly in my opinion

I'm not sure - it is not wrong in the spherical coordinate systems. The plotting just forces a cartesian interpolation which is wrong. But it has to happen before the geographical routines take over, otherwise we have to deal with circular longitudes and wrap around effects...

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 9, 2016

Contributor

The right thing would be to do a spherical interpolation in taup_geo before it is passed to geographiclib then. EDIT: should we open an issue for this?

Also the complete mayavi scene can be composed as pure vtk files. It is then independent from any instabilities and can be displayed e.g. with paraview.

I'm not sure what limitations you might encounter with pure OpenGL. I guess since we use very basic geometries only (lines, spheres, cones), it shouldn't be to difficult to rewrite it. It would probably be more stable but we wouldn't have lights or the whole graphical user interface of mayavi.

webGL on the other hand really adds more fuctionality, so I think it is worth it!

Contributor

MMesch commented Feb 9, 2016

The right thing would be to do a spherical interpolation in taup_geo before it is passed to geographiclib then. EDIT: should we open an issue for this?

Also the complete mayavi scene can be composed as pure vtk files. It is then independent from any instabilities and can be displayed e.g. with paraview.

I'm not sure what limitations you might encounter with pure OpenGL. I guess since we use very basic geometries only (lines, spheres, cones), it shouldn't be to difficult to rewrite it. It would probably be more stable but we wouldn't have lights or the whole graphical user interface of mayavi.

webGL on the other hand really adds more fuctionality, so I think it is worth it!

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 9, 2016

Contributor

One more thing:

the taup_geo routine seems to be somewhat inaccurate for PKIKP phases. E.g. in the video that I have posted, rays that arrive at stations that are close to the antipode do not exactly converge at the station longitude latitude.

I am not sure why this is the case. Either a taup problem, or maybe the ellipticity is not treated correctly ?

Contributor

MMesch commented Feb 9, 2016

One more thing:

the taup_geo routine seems to be somewhat inaccurate for PKIKP phases. E.g. in the video that I have posted, rays that arrive at stations that are close to the antipode do not exactly converge at the station longitude latitude.

I am not sure why this is the case. Either a taup problem, or maybe the ellipticity is not treated correctly ?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Feb 9, 2016

Member

Hmm.. a general comment regarding the ray path plots:

To me those plots seem incredibly overcrowded, I wouldn't know where to start trying to interpret those.. I am not a tomo-guy but I guess this is mostly about knowing where and which rays hit some zone of interest (e.g. CMB). Wouldn't it make more sense to do e.g. a 2D (or 3D) ortho map-plot and plot into it the pierce points (e.g. pierce points at CMB but plotting coastlines over it), labelled by e.g. "{event_id}:{station_code}", or do a heatmap/hexbin of pierce points (to visualize sensitivity of the data set regarding some region and layer of interest)?

Member

megies commented Feb 9, 2016

Hmm.. a general comment regarding the ray path plots:

To me those plots seem incredibly overcrowded, I wouldn't know where to start trying to interpret those.. I am not a tomo-guy but I guess this is mostly about knowing where and which rays hit some zone of interest (e.g. CMB). Wouldn't it make more sense to do e.g. a 2D (or 3D) ortho map-plot and plot into it the pierce points (e.g. pierce points at CMB but plotting coastlines over it), labelled by e.g. "{event_id}:{station_code}", or do a heatmap/hexbin of pierce points (to visualize sensitivity of the data set regarding some region and layer of interest)?

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 9, 2016

Contributor
  1. Don't worry, these plots are using the maximum of phases/stations/events
    possible to put into a single plot. Less paths are way better to see and
    look really great (I'm preparing a video).
  2. I think an ortho map would be really nice to have because it's easier to
    work with it. It is less eye candy/fancy though. The plot right now is more
    for educational purposes and visual impression. There is no reason why we
    shouldn't have both.
Contributor

MMesch commented Feb 9, 2016

  1. Don't worry, these plots are using the maximum of phases/stations/events
    possible to put into a single plot. Less paths are way better to see and
    look really great (I'm preparing a video).
  2. I think an ortho map would be really nice to have because it's easier to
    work with it. It is less eye candy/fancy though. The plot right now is more
    for educational purposes and visual impression. There is no reason why we
    shouldn't have both.
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Feb 9, 2016

Member

There is no reason why we shouldn't have both.

Yeah, that's absolutely right of course.

Member

megies commented Feb 9, 2016

There is no reason why we shouldn't have both.

Yeah, that's absolutely right of course.

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Feb 9, 2016

Member

I'm not saying it'll be any easier, but maybe you should try VisPy too? It supports many backends including WebGL and the notebook without need for any JS (hopefully). Unfortunately, it's not quite as mature as matplotlib, so you definitely need to use a git clone.

Member

QuLogic commented Feb 9, 2016

I'm not saying it'll be any easier, but maybe you should try VisPy too? It supports many backends including WebGL and the notebook without need for any JS (hopefully). Unfortunately, it's not quite as mature as matplotlib, so you definitely need to use a git clone.

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 10, 2016

Contributor

here is a longer video of all kinds of phases:

https://youtu.be/GMrZkqxVc5Q
you might need to choose HD1080p in the bottom right corner

the spherical interpolation problem has to be fixed but otherwise the mayavi part looks fine for me now.

Contributor

MMesch commented Feb 10, 2016

here is a longer video of all kinds of phases:

https://youtu.be/GMrZkqxVc5Q
you might need to choose HD1080p in the bottom right corner

the spherical interpolation problem has to be fixed but otherwise the mayavi part looks fine for me now.

@ThomasLecocq

This comment has been minimized.

Show comment
Hide comment
@ThomasLecocq

ThomasLecocq Feb 10, 2016

Contributor

Wooooow !!!!

Contributor

ThomasLecocq commented Feb 10, 2016

Wooooow !!!!

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 10, 2016

Contributor

@QuLogic btw: do you know the difference between Vispy and Glumpy ? https://glumpy.github.io/

They seem to have the same website :)

Contributor

MMesch commented Feb 10, 2016

@QuLogic btw: do you know the difference between Vispy and Glumpy ? https://glumpy.github.io/

They seem to have the same website :)

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Feb 10, 2016

Member

Vispy is an effort to combine the developments of a couple of other visualization packages: glumpy, pyqtgraph, galry, and some others I probably forgot. Its still in a pretty early stage but there was a nice talk at last year's scipy: https://www.youtube.com/watch?v=_3YoaeoiIFI

Member

krischer commented Feb 10, 2016

Vispy is an effort to combine the developments of a couple of other visualization packages: glumpy, pyqtgraph, galry, and some others I probably forgot. Its still in a pretty early stage but there was a nice talk at last year's scipy: https://www.youtube.com/watch?v=_3YoaeoiIFI

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Feb 10, 2016

Member

Nice video! 👍

Member

megies commented Feb 10, 2016

Nice video! 👍

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 18, 2016

Contributor

I am just thinking how to continue. I structure the plotting routines of inventory and catalog better and to include the ray path plotting routine, I will add an 'get_all' function to inventory and catalog. This could be called as:

long_list = catalog.get_all(['latitude', 'longitude', 'depth', 'focal_mechanism'])

this would replace the current

for network in inventory:
    for station in network:
         lats.append(station.latitude)
         [...]

loops in the plotting functions and is also useful for other functions that need bulk inventory/catalog data. Tell me if you have a better idea...

Contributor

MMesch commented Feb 18, 2016

I am just thinking how to continue. I structure the plotting routines of inventory and catalog better and to include the ray path plotting routine, I will add an 'get_all' function to inventory and catalog. This could be called as:

long_list = catalog.get_all(['latitude', 'longitude', 'depth', 'focal_mechanism'])

this would replace the current

for network in inventory:
    for station in network:
         lats.append(station.latitude)
         [...]

loops in the plotting functions and is also useful for other functions that need bulk inventory/catalog data. Tell me if you have a better idea...

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Feb 18, 2016

Member

We could also do a slightly more generic variant:

catalog.get_all(('preferred_origin.latitude', 'preferred_origin.longitude',
                 'preferred_focal_mechanism.mt.tensor', ...))

Which would return lists of dictionaries with the requested keys. I'm not sure if we need the preferred_ prefix or not but I think it might be useful. For inventories this is a bit more complex as they are nested. We could also think to add a more generic query interface that would replace the current filter() methods. I did something along these lines here which I think has quite a nice interface: http://seismicdata.github.io/pyasdf/querying_data.html

I also want to add a couple of more things. I'll do it after the release:

Member

krischer commented Feb 18, 2016

We could also do a slightly more generic variant:

catalog.get_all(('preferred_origin.latitude', 'preferred_origin.longitude',
                 'preferred_focal_mechanism.mt.tensor', ...))

Which would return lists of dictionaries with the requested keys. I'm not sure if we need the preferred_ prefix or not but I think it might be useful. For inventories this is a bit more complex as they are nested. We could also think to add a more generic query interface that would replace the current filter() methods. I did something along these lines here which I think has quite a nice interface: http://seismicdata.github.io/pyasdf/querying_data.html

I also want to add a couple of more things. I'll do it after the release:

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Feb 18, 2016

Member

I did something along these lines here which I think has quite a nice interface: http://seismicdata.github.io/pyasdf/querying_data.html

Just a quick glimpse but I like that comparison style way more than our current hacky catalog filter or list-of-strings controlled selection criteria..

Member

megies commented Feb 18, 2016

I did something along these lines here which I think has quite a nice interface: http://seismicdata.github.io/pyasdf/querying_data.html

Just a quick glimpse but I like that comparison style way more than our current hacky catalog filter or list-of-strings controlled selection criteria..

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 19, 2016

Contributor

ok that looks much nicer. Any chance that you can directly transfer this ifilter method to obspy? I think the problem with catalog and inventory is that they are deeply nested structures which makes it difficult to find stuff if you are not an expert. A convenience extract method should return something more chaotic but less deeply nested.

Contributor

MMesch commented Feb 19, 2016

ok that looks much nicer. Any chance that you can directly transfer this ifilter method to obspy? I think the problem with catalog and inventory is that they are deeply nested structures which makes it difficult to find stuff if you are not an expert. A convenience extract method should return something more chaotic but less deeply nested.

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 19, 2016

Contributor

another question: Is there any way to refer to a specific station and a specific event inside an inventory or catalog? E.g. for the path routine, one could just return the id of the event and station for each path instead of specific latitude/longitudes and the plotting routines will extract whatever they need.

Contributor

MMesch commented Feb 19, 2016

another question: Is there any way to refer to a specific station and a specific event inside an inventory or catalog? E.g. for the path routine, one could just return the id of the event and station for each path instead of specific latitude/longitudes and the plotting routines will extract whatever they need.

MMesch and others added some commits Jul 8, 2016

remove mayavi 3d ray path plotting again
kind of was decided that this should be moved to a separate repository,
as it's pretty heavy and unstable in CI and testing
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 11, 2017

Member

I've added a small test for the path resampling, rebased and force pushed.

I remove the vtk output, @MMesch if you offer to add a test for it I would consider keeping it.

Otherwise I'll improve the test "test_compute_ray_paths" maybe in the evening because right now it's hardly testing anything, just asserting that a list with one item is returned.

Member

megies commented Oct 11, 2017

I've added a small test for the path resampling, rebased and force pushed.

I remove the vtk output, @MMesch if you offer to add a test for it I would consider keeping it.

Otherwise I'll improve the test "test_compute_ray_paths" maybe in the evening because right now it's hardly testing anything, just asserting that a list with one item is returned.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 11, 2017

Member

@MMesch next time please take some time to implement a real test. This is really not cutting it:
https://github.com/obspy/obspy/pull/1236/files#diff-dd572eba2d259313f2468ec5ae6d42d9R56
screenshot from 2017-10-11 18-06-14

Member

megies commented Oct 11, 2017

@MMesch next time please take some time to implement a real test. This is really not cutting it:
https://github.com/obspy/obspy/pull/1236/files#diff-dd572eba2d259313f2468ec5ae6d42d9R56
screenshot from 2017-10-11 18-06-14

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Oct 11, 2017

Contributor
Contributor

MMesch commented Oct 11, 2017

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Oct 12, 2017

Contributor

Also it seems like bad design to include spaces for padding in the textual headers of the paths.. this should be done in the plotting part..

agree that's better

can you elaborate on the significance of always adding 0.0 to the output tuple

originally, I wanted to leave the routine flexible to add a value such as the amplitude from the radiation pattern to the ray path. This can be removed or organized in a better way if you have a better idea!

Contributor

MMesch commented Oct 12, 2017

Also it seems like bad design to include spaces for padding in the textual headers of the paths.. this should be done in the plotting part..

agree that's better

can you elaborate on the significance of always adding 0.0 to the output tuple

originally, I wanted to leave the routine flexible to add a value such as the amplitude from the radiation pattern to the ray path. This can be removed or organized in a better way if you have a better idea!

megies added some commits Oct 12, 2017

adjust get_ray_paths routine
add event and origin id to output, remove leading spaces from labels of
rays (station code and basic event info string)
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 12, 2017

Member

I hope this should do it now.. added some proper tests and sanitized output of get_ray_paths some

Member

megies commented Oct 12, 2017

I hope this should do it now.. added some proper tests and sanitized output of get_ray_paths some

outdated

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 12, 2017

Member

Looks like CI is gonna give the nod.. @krischer @MMesch I think this is good to go when Travis completes.

Member

megies commented Oct 12, 2017

Looks like CI is gonna give the nod.. @krischer @MMesch I think this is good to go when Travis completes.

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Oct 12, 2017

Contributor
Contributor

MMesch commented Oct 12, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 12, 2017

Member

The question is whether we put the mayavi/vtk moment tensor also in a separate repository

Sure, go ahead.

Member

megies commented Oct 12, 2017

The question is whether we put the mayavi/vtk moment tensor also in a separate repository

Sure, go ahead.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 13, 2017

Member

Gonna push one additional minor change and will merge this once CI goes green then.

Member

megies commented Oct 13, 2017

Gonna push one additional minor change and will merge this once CI goes green then.

@krischer

Minor comments. I can also do it if you are currently not working on it.

Show outdated Hide outdated obspy/core/util/testing.py
Show outdated Hide outdated obspy/imaging/tests/data/IU_stations.txt

megies added some commits Oct 13, 2017

image testing: remove handling of alpha channels again
currently not used in any test
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 13, 2017

Member

Should be ready now.. gonna merge when CI goes green.

Member

megies commented Oct 13, 2017

Should be ready now.. gonna merge when CI goes green.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 13, 2017

Member

I'll merge this locally and squash everything into one commit, to avoid polluting our master history with all the binary stuff. The history and full diff across individual commits can then still be looked up here and at https://github.com/mmesch/obspy/tree/gc_paths

Member

megies commented Oct 13, 2017

I'll merge this locally and squash everything into one commit, to avoid polluting our master history with all the binary stuff. The history and full diff across individual commits can then still be looked up here and at https://github.com/mmesch/obspy/tree/gc_paths

megies added a commit that referenced this pull request Oct 13, 2017

Merge pull request #1236 from MMesch/gc_paths
(locally squashed into a single commit)
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 13, 2017

Member

This was merged locally after squashing history, see 6b43db0.

Member

megies commented Oct 13, 2017

This was merged locally after squashing history, see 6b43db0.

@megies megies closed this Oct 13, 2017

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