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

Add more features to nordic IO #1924

Merged
merged 22 commits into from Mar 26, 2018

Conversation

Projects
None yet
4 participants
@calum-chamberlain
Contributor

calum-chamberlain commented Oct 12, 2017

Work in progress, do not merge yet!

What does this PR do?

This PR attempts to add more functionality to Nordic format IO operations. This will add in:
- [x] Read hyp error estimates and convert to confidenceEllipsoid;
- [x] Write confidenceEllipsoid to hyp uncertainty estimates;

  • Read focal mechanism solutions;
  • Write focal mechanism solutions;
  • Read moment tensor solutions;
  • Write moment tensor solutions.
  • latin1 encoded reading (currently raises a UnicodeDecodeError in Py3 and other errors in py2) - how should format be detected, chardet, or should the user specify if there is an error? Could ask Jens what formats are accepted and try then?
    - [ ] More xyz_to_confidence_ellipsoid and confidence_ellipsoid_to_xyz to obspy.core.event.origin.
  • Review comments.

Why was it initiated? Any relevant Issues?

The ability to read Hyp derived errors was requested by @shicks-seismo in issue #1909

PR Checklist

  • This PR is not directly related to an existing issue (which has no PR yet).
  • 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 .
@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Oct 12, 2017

Contributor

This PR is more to document progress, and ask for help on converting between covariance matrix and confidenceEllipsoid (and back). I have made a stab at this in obspy.io.nordic.core.xyz_to_confidence_ellipsoid and the sister function: obspy.io.nordic.core.confidence_ellipsoid_to_xyz, but I'm failing at the moment - welcome any changes here!

I don't think this PR is urgent.

Contributor

calum-chamberlain commented Oct 12, 2017

This PR is more to document progress, and ask for help on converting between covariance matrix and confidenceEllipsoid (and back). I have made a stab at this in obspy.io.nordic.core.xyz_to_confidence_ellipsoid and the sister function: obspy.io.nordic.core.confidence_ellipsoid_to_xyz, but I'm failing at the moment - welcome any changes here!

I don't think this PR is urgent.

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Oct 13, 2017

Contributor

Reading now implemented, all tests pass expect confidenceEllipsoid conversion, which I will work to fix at some point in the next couple of weeks.

Contributor

calum-chamberlain commented Oct 13, 2017

Reading now implemented, all tests pass expect confidenceEllipsoid conversion, which I will work to fix at some point in the next couple of weeks.

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Oct 31, 2017

Contributor

@shicks-seismo, I think the reading in is working, do you have any expected results for confidence ellipsoids from x, y, z and covariance? I'm still stuck on writing (going back the other way), I can get the right numbers but not the right order. Any help appreciated.

Contributor

calum-chamberlain commented Oct 31, 2017

@shicks-seismo, I think the reading in is working, do you have any expected results for confidence ellipsoids from x, y, z and covariance? I'm still stuck on writing (going back the other way), I can get the right numbers but not the right order. Any help appreciated.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Nov 1, 2017

Member

This PR is more to document progress, and ask for help on converting between covariance matrix and confidenceEllipsoid (and back). I have made a stab at this in obspy.io.nordic.core.xyz_to_confidence_ellipsoid and the sister function: obspy.io.nordic.core.confidence_ellipsoid_to_xyz, but I'm failing at the moment - welcome any changes here!

I think these helper functions should eventually be moved to obspy.core.event.origin for general use. No hurry though, can also be refactored at the end when everything is ready, and we won't do 1.2.0 until march/april.

Member

megies commented Nov 1, 2017

This PR is more to document progress, and ask for help on converting between covariance matrix and confidenceEllipsoid (and back). I have made a stab at this in obspy.io.nordic.core.xyz_to_confidence_ellipsoid and the sister function: obspy.io.nordic.core.confidence_ellipsoid_to_xyz, but I'm failing at the moment - welcome any changes here!

I think these helper functions should eventually be moved to obspy.core.event.origin for general use. No hurry though, can also be refactored at the end when everything is ready, and we won't do 1.2.0 until march/april.

@shicks-seismo

This comment has been minimized.

Show comment
Hide comment
@shicks-seismo

shicks-seismo Nov 7, 2017

Member

Hi @calum-chamberlain, sorry for the late reply. I am not really using the error ellipsoids so I don't really have any expectations here.
Note my new PR @ #1991 regarding longitude and depth reading.

Member

shicks-seismo commented Nov 7, 2017

Hi @calum-chamberlain, sorry for the late reply. I am not really using the error ellipsoids so I don't really have any expectations here.
Note my new PR @ #1991 regarding longitude and depth reading.

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Jan 24, 2018

Contributor

Brief update on this: I have been in touch with Jens Havskov and he thinks they (the SEISAN team) have conversions from x, y, z to ellipse (oid?), but not the reverse, which is still the part I am struggling with. He also tried out a nordic file with some interesting things that I didn't account for, but have included here.

One other thing his file highlighted is that this module doesn't currently account for different encodings - his file was encoded as latin1, which breaks these functions. Do we have a good/standard way of checking encoding (chardet seems like the tool, but would slow things down most of the time)? As a prefred way of doing it, I have asked Jens what the standard encodings are and then I can add in a try: .. except: clause to try different encodings. Thoughts @krischer @megies ?

Contributor

calum-chamberlain commented Jan 24, 2018

Brief update on this: I have been in touch with Jens Havskov and he thinks they (the SEISAN team) have conversions from x, y, z to ellipse (oid?), but not the reverse, which is still the part I am struggling with. He also tried out a nordic file with some interesting things that I didn't account for, but have included here.

One other thing his file highlighted is that this module doesn't currently account for different encodings - his file was encoded as latin1, which breaks these functions. Do we have a good/standard way of checking encoding (chardet seems like the tool, but would slow things down most of the time)? As a prefred way of doing it, I have asked Jens what the standard encodings are and then I can add in a try: .. except: clause to try different encodings. Thoughts @krischer @megies ?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jan 24, 2018

Member

One other thing his file highlighted is that this module doesn't currently account for different encodings - his file was encoded as latin1, which breaks these functions. Do we have a good/standard way of checking encoding (chardet seems like the tool, but would slow things down most of the time)? As a prefred way of doing it, I have asked Jens what the standard encodings are and then I can add in a try: .. except: clause to try different encodings. Thoughts @krischer @megies ?

without looking at the details.. if there really are different encodings used in the wild, the easiest option would be to at least add an encoding kwarg that can be specified by the user (maybe defaulting to ASCII), so at least users can read such files without hacking the code

Member

megies commented Jan 24, 2018

One other thing his file highlighted is that this module doesn't currently account for different encodings - his file was encoded as latin1, which breaks these functions. Do we have a good/standard way of checking encoding (chardet seems like the tool, but would slow things down most of the time)? As a prefred way of doing it, I have asked Jens what the standard encodings are and then I can add in a try: .. except: clause to try different encodings. Thoughts @krischer @megies ?

without looking at the details.. if there really are different encodings used in the wild, the easiest option would be to at least add an encoding kwarg that can be specified by the user (maybe defaulting to ASCII), so at least users can read such files without hacking the code

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Jan 24, 2018

Member

I think trying a few default encodings would be good and simple first step. Otherwise you could raise an error and tell users that they have to specify the encoding and maybe even point them to the chardet library to figure out which encoding they have.

Member

krischer commented Jan 24, 2018

I think trying a few default encodings would be good and simple first step. Otherwise you could raise an error and tell users that they have to specify the encoding and maybe even point them to the chardet library to figure out which encoding they have.

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Jan 24, 2018

Contributor

The word from Jens is that nordic files in seisan are read in as bytes and decoded as single-byte characters, which seems to be consistent with everything being latin-1 encoded. This appears to only have been an issue for some of the less-used characters (like: Ø, in the included dos-file.sfile test file).

Reading everything using:

f = open(sfile, 'r', encoding='latin-1')

seems to work fine and pass all my tests...

Contributor

calum-chamberlain commented Jan 24, 2018

The word from Jens is that nordic files in seisan are read in as bytes and decoded as single-byte characters, which seems to be consistent with everything being latin-1 encoded. This appears to only have been an issue for some of the less-used characters (like: Ø, in the included dos-file.sfile test file).

Reading everything using:

f = open(sfile, 'r', encoding='latin-1')

seems to work fine and pass all my tests...

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Jan 24, 2018

Member

Okay good. I still think an optional encoding argument would be useful for example when somebody manually edits a file and accidentally saves it in a different encoding. Also its really trivial to implement.

Member

krischer commented Jan 24, 2018

Okay good. I still think an optional encoding argument would be useful for example when somebody manually edits a file and accidentally saves it in a different encoding. Also its really trivial to implement.

@krischer

Thanks a lot for this - it definitely was a lot of work! Mostly some minor comments from my side - otherwise this looks pretty good.

I would really like to see a few tests cases for the confidence ellipsoid conversions. I think it should be pretty straightforward to at least test some simple axes-aligned cases to catch potential basic errors. Rotations are always a great pain to test :(

Show outdated Hide outdated obspy/io/nordic/core.py
Show outdated Hide outdated CHANGELOG.txt
Show outdated Hide outdated obspy/io/nordic/tests/test_nordic.py
Show outdated Hide outdated obspy/io/nordic/tests/test_nordic.py
Show outdated Hide outdated obspy/io/nordic/tests/test_nordic.py
Show outdated Hide outdated obspy/io/nordic/core.py
Show outdated Hide outdated obspy/io/nordic/core.py
Show outdated Hide outdated obspy/io/nordic/core.py
Show outdated Hide outdated obspy/io/nordic/core.py
Generate the two lines required for moment tensor solutions in Nordic.
"""
# First line contains hypocenter info, second contains tensor info
lines = [list(' ' * 79 + 'M'), list(' ' * 79 + 'M')]

This comment has been minimized.

@krischer

krischer Jan 25, 2018

Member

I've not seen this before but its actually kind of a nice way to get a mutable string 😄

@krischer

krischer Jan 25, 2018

Member

I've not seen this before but its actually kind of a nice way to get a mutable string 😄

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Jan 25, 2018

Contributor

Thanks @krischer I will get on those before merging (as well as actually finished the confidence ellipsoid functions).

Contributor

calum-chamberlain commented Jan 25, 2018

Thanks @krischer I will get on those before merging (as well as actually finished the confidence ellipsoid functions).

calum-chamberlain added some commits Jan 28, 2018

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Feb 1, 2018

Contributor

I think I might have got it working - I think I was using the wrong form of the rotation matrix for Tait-Bryon angles. I was using the ZYX form (which I was led to from the QuakeML description), but it looks like I was going the wrong-way, so changing to XYZ appears to give the correct answers for the simple cases I have tested.

Contributor

calum-chamberlain commented Feb 1, 2018

I think I might have got it working - I think I was using the wrong form of the rotation matrix for Tait-Bryon angles. I was using the ZYX form (which I was led to from the QuakeML description), but it looks like I was going the wrong-way, so changing to XYZ appears to give the correct answers for the simple cases I have tested.

calum-chamberlain added some commits Feb 1, 2018

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Feb 7, 2018

Contributor

@megies and @krischer, is there anything else you would like me to change in this? From my side it looks good to go, but I haven't moved the confidence-ellipsoid functions to somewhere more general - I didn't see any functions in core.event.Event, so thought I might have been looking in the wrong place.

@megies, I didn't understand your comment on the nested get operations, happy to change how that is implemented though if needed?

Contributor

calum-chamberlain commented Feb 7, 2018

@megies and @krischer, is there anything else you would like me to change in this? From my side it looks good to go, but I haven't moved the confidence-ellipsoid functions to somewhere more general - I didn't see any functions in core.event.Event, so thought I might have been looking in the wrong place.

@megies, I didn't understand your comment on the nested get operations, happy to change how that is implemented though if needed?

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 1, 2018

Member

Sorry for the delay. Can be merged from my point of view. @megies what do you think?

Member

krischer commented Mar 1, 2018

Sorry for the delay. Can be merged from my point of view. @megies what do you think?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 2, 2018

Member

I didn't look at every detail but I guess it's OK.. personally, I'd move the confidence ellipsoid routine to core.event so that it can be reused more easily..

Member

megies commented Mar 2, 2018

I didn't look at every detail but I guess it's OK.. personally, I'd move the confidence ellipsoid routine to core.event so that it can be reused more easily..

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 2, 2018

Member

I agree with moving the confidence ellipsoid routine as it might be useful to have around in a more accessible place.

Member

krischer commented Mar 2, 2018

I agree with moving the confidence ellipsoid routine as it might be useful to have around in a more accessible place.

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Mar 21, 2018

Contributor

Hi all. Moved confidence ellipsoid things and added more tests, which showed that for more complicated cases it isn't working. I have failed to fix it, and haven't been able to get help. I suggest that I take out the confidence ellipsoid conversions and leave this with all the other little patches to nordic.
Sorry for taking up so much time.

Contributor

calum-chamberlain commented Mar 21, 2018

Hi all. Moved confidence ellipsoid things and added more tests, which showed that for more complicated cases it isn't working. I have failed to fix it, and haven't been able to get help. I suggest that I take out the confidence ellipsoid conversions and leave this with all the other little patches to nordic.
Sorry for taking up so much time.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 21, 2018

Member

The confidence ellipsoid stuff would've been nice, but yeah, whatever works for you.. you're doing the work in here.. ;-)

Member

megies commented Mar 21, 2018

The confidence ellipsoid stuff would've been nice, but yeah, whatever works for you.. you're doing the work in here.. ;-)

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Mar 26, 2018

Contributor

I've removed the confidence ellipsoid things - I will work on them outside of a PR for a while and see where I get, but in the mean-time I think the things in here should be included in obspy. Looks like appveyor fails are not from my things. Good to go AFAIK, sans confidenceEllipsoid conversion.

Contributor

calum-chamberlain commented Mar 26, 2018

I've removed the confidence ellipsoid things - I will work on them outside of a PR for a while and see where I get, but in the mean-time I think the things in here should be included in obspy. Looks like appveyor fails are not from my things. Good to go AFAIK, sans confidenceEllipsoid conversion.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Mar 26, 2018

Member

I think the things in here should be included in obspy

I agree - this has been ready for a while aside the confidence ellipsoid. Let's get this merged. As always: thanks a bunch!

Looks like appveyor fails are not from my things.

Yep - this being worked on here: #2089

Member

krischer commented Mar 26, 2018

I think the things in here should be included in obspy

I agree - this has been ready for a while aside the confidence ellipsoid. Let's get this merged. As always: thanks a bunch!

Looks like appveyor fails are not from my things.

Yep - this being worked on here: #2089

@krischer krischer merged commit 39f063d into master Mar 26, 2018

2 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@krischer krischer deleted the nordic-patch branch Mar 26, 2018

@krischer krischer changed the title from WIP: Add more features to nordic IO to Add more features to nordic IO Mar 26, 2018

megies added a commit that referenced this pull request Jul 9, 2018

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