Skip to content
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

Added error ellipses and high-accuracy hypocenters to nordic format #2451

Open
wants to merge 18 commits into
base: master
from

Conversation

@WayneCrawford
Copy link

commented Sep 12, 2019

What does this PR do?

Converts nordic location uncertainties (covariance matrices) to QuakeML style (error ellipses). Note that it only does this in the X-Y direction (2D, ellipse), not 3D (ellipsoid).
Also now reads high-accuracy hypocenter lines (type 'H') from Nordic files.

Why was it initiated? Any relevant Issues?

I needed to convert nordic files to QuakeML and saw that some important information
was missing:

  • The uncertainty information (stored as covariance matrix elements in Nordic and as
    error ellipses in QuakeML)
  • The high accuracy epicenter time/positions specified on Nordic type 'H' lines

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+ DOCS"
  • 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 .
@WayneCrawford WayneCrawford requested a review from calum-chamberlain Sep 12, 2019
@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Thanks for this @WayneCrawford! I will try and have a look at this today. After a quick glance this looks super useful, but I have a couple of questions:

  1. I wonder if we need all the events in the "high accuracy" test file? Could we get away with a smaller file, just to save some space?
  2. Do you have tests for the methods in the ellipse class?
  3. Have you implemented the reverse conversion (e.g. QuakeML style error ellipse to Nordic format) for writing errors to Nordic?

I realised a few weeks back that the event ID wasn't being read either, and it really should be. I should probably open another PR for that.

@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Sep 12, 2019

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2019

Thanks @WayneCrawford:

  1. No worries, I make beginner mistakes all the time - still learning heaps and I don't think you ever stop. I will let you trim the file to just that first event then;
  2. I will have a look at how much that test covers; it might be good to have some more tests for this with some expected range of error ellipses to make sure that we cope with all sorts of different ellipses.
  3. It would be awesome if you can add this! This would probably make testing simpler as well: you can add tests to ensure the results are consistent (this was what I struggled with when trying to do this before, but your implementation looks much better than mine was), and test over a range of possible angles.
…ellipse

to test several cases and compare forward and reverse ellipse calculations.
@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Sep 13, 2019

OK, I added a _ellipse.to_uncerts() and .to_cov() methods and added another test that covers a wider variety of covariance matrixes (test_uncert_ellipse). I didn't implement the input of uncertainties from QuakeML because you already have a routine that does this from the confidence ellipsoid (which is more complete than the ellipse): in core._write_hyp_error_line(). If that routine fails perhaps we should use my routine as a backup, but I don't know how to implement that.

Copy link
Contributor

left a comment

Thanks for this @WayneCrawford, sorry I got distracted over the weekend and didn't get around to this. There are a few syntax things that circleci will be complaining about that need to be fixed. I also had some other comments, which mostly relate to my ugly code (sorry).

I didn't realise that this was just 2D, but I think that is worthwhile! Unfortunately the call to confidence_ellipsoid_to_xyz() was something that was left in after my attempt at 3D, so this doesn't exist. We should put your function in there instead.

obspy/io/nordic/core.py Outdated Show resolved Hide resolved
obspy/io/nordic/core.py Outdated Show resolved Hide resolved
obspy/io/nordic/core.py Show resolved Hide resolved
obspy/io/nordic/core.py Outdated Show resolved Hide resolved
obspy/io/nordic/core.py Show resolved Hide resolved
obspy/io/nordic/ellipse.py Outdated Show resolved Hide resolved
obspy/io/nordic/ellipse.py Show resolved Hide resolved
obspy/io/nordic/ellipse.py Outdated Show resolved Hide resolved
obspy/io/nordic/ellipse.py Outdated Show resolved Hide resolved
…_errors

and depth_errors; used more robust _float_conv() and _int_conv() rather
than float() and int() when reading from text files. Made nordic/ellipse.py
docstrings consistent with obspy
@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Sep 24, 2019

Hi Calum,

What is the status of this add-on? I see fail checks but they seem to apply to elements that I haven't touched (there seems to be some bad db, taup, signal, imaging codes according to circleCI at least) and most of the remaining changes requested by circleci to my part are rejected by my flake8!

Maybe I should merge with a more recent build and then resubmit?

Wayne

@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Sep 24, 2019

I just tried merging:

git fetch upstream
git checkout master
git merge upstream/master

which caught me up with the things modified in master over the past 12 days, but it didn't get rid of the files that seem to be causing problems with the checks. Whenever I run git status, in addition to showing the files I really modified, it also lists some other files that I haven't touched (see attached figure). When I try to get rid of them (using git checkout -- obspy/signal/tests/data/KARC.LHZ.SAC.asc.gz, for example) they stay there.

My only guess is that they are binary files that my Mac treats/stores differently from Linux machines and so git sees some difference in them. But I don't know why git checkout --
doesn't do anything. Any advice on what to do?

Screen Shot 2019-09-24 at 12 48 54

@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Sep 24, 2019

The problem with dos-file.sfile was/is also an encoding issue (it seems that importing to a Mac replaces CRLFs with LFs).

git diff with a pre-me commit gives:

0> git diff f7fbb91a42d148063ada64ef809f4f39ef569b9c obspy/io/nordic/tests/data/dos-file.sfile 
warning: CRLF will be replaced by LF in obspy/io/nordic/tests/data/dos-file.sfile.
The file will have its original line endings in your working directory.

which, by the way, was the warning for all of those obspy/{signal,taup,...} files that show up as modified when I do git status

I tried to revert to the original version:

git checkout f7fbb91a42d148063ada64ef809f4f39ef569b9c obspy/io/nordic/tests/data/dos-file.sfile

after which obspy/io/nordic/tests/data/dos-file.sfile did appear as modified in git status. But when I ran git add obspy/io/nordic/tests/data/dos-file.sfile, I got the following message:

warning: CRLF will be replaced by LF in obspy/io/nordic/tests/data/dos-file.sfile.
The file will have its original line endings in your working directory.

and a subsequent git status no longer showed the difference and git commit said "your branch is up to date"

To make a long story short, I should have excluded dos-file.sfile from my first commit, but now that I haven't I can't go back. You will have to reject this file's changes, or run a small merge-push after accepting my mods. I don't know what's going on with the other binary files that I referred to in the comment above: I don't think I ever added them but they seem to be what circleci is complaining about: is that because my "push" puts them on GitHub anyway even if I don't commit them?

Wayne

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

commented Sep 27, 2019

Hi @WayneCrawford, sorry for being so slow!

The problem with the binary files might be changed modes on checkout - you can check that by running git diff on one of the files. If it is that, then you can configure git to ignore mode changes.

I will have to run that small change later for the DOS file - should be okay.

I will respond to the rest in-line and have a quick re-review now. Sorry again for being so slow!

Copy link
Contributor

left a comment

There are three minor syntax things that are resulting in the fails. Can you fix them, then the tests will run? Other things look good, but I will have a proper look once the tests all run.

.gitattributes to ignore PNGs for CFLF problems ('*.png binary')
@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Sep 27, 2019

CircleCi now works and so does the full build test (Appveyor) for Python 2.7, but Apveyor fails for Python 3.6, 32-bit. I couldn't find anything specifically related to the new code in that failure, could you help me figure the problem out?

By the way, I was able to get rid of most of the extra files listing in git status by declaring them as binary in .gitattributes. Of course, this changes .gitattributes, which I don't think Lion or Tobias would appreciate, so I git checkout -- .gitattributes. Seems like a dirty workaround, there must be some global git command that force my computer (OS X) not to look at "line endings" in binary files!

@megies

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

@WayneCrawford see #2461 the broken builds on Travis minimum deps and Appveyor 32bit are unrelated, ignore those two

@megies

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

@WayneCrawford what's your git version? I kind of remember old git versions having such quirks, wasn't there something like that at some point @trichter?

@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Sep 27, 2019

@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Sep 27, 2019

@trichter

This comment has been minimized.

Copy link
Member

commented Sep 29, 2019

@WayneCrawford what's your git version? I kind of remember old git versions having such quirks, wasn't there something like that at some point @trichter?

Yes, I had the same problem. See #1978. I solved it by upgrading git. The problem was introduced by #1964, therein it is stated that git version needs to be at least 2.10. My old git version was 2.7.4, too, but on Linux.

How does git generally decide that a file is binary? If it's based on the executable bit, these files definitely don't have that bit set.

Probably binary here means not human readable rather than executable, no idea how git decides that, obviously these old git versions get confused.

Seems like a dirty workaround, there must be some global git command that force my computer (OS X) not to look at "line endings" in binary files!

You could try this setting:

$ git config --global core.autocrlf false    

which can be found on stackoverflow.

@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Sep 30, 2019

Updating my git version seems to have gotten rid of the binary file problems.

@megies

This comment has been minimized.

Copy link
Member

commented Sep 30, 2019

Uhm @WayneCrawford 2.7 is from 2015, current git is 2.23 ;-)

@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Sep 30, 2019

@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Oct 10, 2019

Hi,
What is the status of this pull request? I think I got everything to work except the 32bit builds, which Tobias said is normal.

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

@WayneCrawford sorry, I'm on fieldwork at the moment: it looked good to me the last time I looked, and if everything is passing I think I'm happy.

@WayneCrawford

This comment has been minimized.

Copy link
Author

commented Oct 11, 2019

Copy link
Author

left a comment

Sorry, it took me a while to figure out that I had to review this. All seem good here. Calum will have to put the DOS-style version of dos-file.sfile back in afterwards

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.