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

Nordic high accuracy #2351

Merged
merged 7 commits into from Mar 15, 2019

Conversation

@calum-chamberlain
Copy link
Contributor

calum-chamberlain commented Mar 11, 2019

What does this PR do?

Adds support for high(er)-precision timing in Nordic pick files (closes #2348) and add support for long phase names - from the seisan manual:

Long phase names: An 8 character phase can be used in column 11-18. There is then not room for polarity information. the weight is then put into column 9. This format is recognised by HYP and MULPLT

Why was it initiated? Any relevant Issues?

Seconds can run in columns 23-28 - I misread the indexing and was reading columns 23-28 rather than 22-28. #2348 pointed out that this is used when seisan is run in "high-accuracy" mode.

Note, pick times are now written out by default in high-accuracy from obspy now. I need to confirm that seisan is always happy with this, if not then a flag option for high-accuracy should be implemented.

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 .
Cope with seconds overflow and fix linting
Issue #2348 highlighted that seconds in NORDIC format can overflow
from the stated block into column 29.  This commit allows for this
and enforces reading coda duration from column 29 onwards, rather
than column 28 onwards (as was previously done because 29 was
supposed to be empty). A test file for this was added.

@calum-chamberlain calum-chamberlain self-assigned this Mar 12, 2019

@calum-chamberlain calum-chamberlain added this to the 1.2.0 milestone Mar 12, 2019

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor Author

calum-chamberlain commented Mar 12, 2019

I checked whether SEISAN was happy reading high-accuracy phase-picks when not run in high-accuracy mode, and it seems to be fine. Nevertheless a flag has been added for writing to turn off high accuracy mode (which is also tested).

RE tests:

circleci pep8 tests are not related to files I have changed for this:

obspy/core/util/misc.py:746:29: E117 over-indented
obspy/imaging/waveform.py:1481:13: E117 over-indented

and it looks like appveyor is failing on response removal things that are unrelated.

I think this is ready for review...

@d-chambers
Copy link
Member

d-chambers left a comment

This looks good. It looks like you have comprehensively tested the new functionality.

One thing that might help the maintainability would be to move some of the utility functions (like string converters) into a obspy.io.seisan.util module as is done in many of the other IO modules. Core is getting rather long, and there has been some talk of trying to unify the io utils at some point in time.

Show resolved Hide resolved 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
Response to review
1. Removed used_station_count comment and added a doc-string note
   stating that it had been moved;
2. Refactored some utility functions into a utils submodule;
3. Changed accepted line-tags to a tuple to avoid mutation.
@calum-chamberlain

This comment has been minimized.

Copy link
Contributor Author

calum-chamberlain commented Mar 13, 2019

I added a doc-string for the change from comment to the correct place in origin.quality.used_station_count. +DOCS

@calum-chamberlain

This comment has been minimized.

Copy link
Contributor Author

calum-chamberlain commented Mar 14, 2019

Docs built fine, the only thing I changed is in the Changed in version of this page.

Test fails on appveyor are for trace comparison after response removal, the tests that fail are:

  • FAIL: test_remove_response (obspy.core.tests.test_stream.StreamTestCase)
  • FAIL: test_simulate_seedresp_parser (obspy.core.tests.test_stream.StreamTestCase)

Looks like the version of flake8 installed on circleci has been bumped from 3.6.0 to 3.7.7, which is resulting in a lot of import naming related complaints. Happy to trudge through marking those to be ignored, or changing them today if it helps (in another PR)?

The travis fail looked like a network issue, I have restarted it.

Good to go @d-chambers ?

@d-chambers

This comment has been minimized.

Copy link
Member

d-chambers commented Mar 14, 2019

Looks good to me, feel free to merge

@calum-chamberlain calum-chamberlain added this to Waiting for Review in Release 1.2.0 Mar 14, 2019

@calum-chamberlain calum-chamberlain moved this from Waiting for Review to Waiting for final manual validation by Core Dev in Release 1.2.0 Mar 14, 2019

@@ -714,7 +574,11 @@ def _read_picks(tagged_lines, new_event):
if len(line) < 80:
line = line.ljust(80) # Pick-lines without a tag may be short.
weight = line[14]
if weight == '_':
if weight not in ' 012349_': # Long phase name

This comment has been minimized.

@megies

megies Mar 15, 2019

Member

This might be good to have as a CONSTANT at the top like the other header thingies (e.g. ALLOWED_WEIGHTS)

But ignore for now, we don't want another iteration of this PR for reviewing to speed up things.. :-)

@megies

This comment has been minimized.

Copy link
Member

megies commented Mar 15, 2019

Tests pass and I don't know enough of the nordic implementation details anyway, so I'm happy to follow the previous review, thanks @calum-chamberlain and @d-chambers!

(Test fails unrelated)

@megies megies merged commit ab55fe0 into master Mar 15, 2019

1 of 5 checks passed

ci/circleci Your tests failed on CircleCI
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
docs-buildbot Build succeeded, but there are warnings/errors:
Details
docker-testbot docker testbot results not available yet
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@megies megies deleted the nordic-high-accuracy branch Mar 15, 2019

@megies megies moved this from Waiting for final manual validation by Core Dev to Done in Release 1.2.0 Mar 15, 2019

@megies

This comment has been minimized.

Copy link
Member

megies commented Mar 15, 2019

Oh @calum-chamberlain issue numbers in changelog missing, but I'll add them on master now, so no worries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.