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: Bug-fix amplitude and wavefile write #2021

Merged
merged 6 commits into from Apr 23, 2018

Conversation

Projects
None yet
5 participants
@calum-chamberlain
Contributor

calum-chamberlain commented Dec 5, 2017

  • Wavefile lines were not correctly justified when full paths were given
  • amplitudes were not handled when magnitude_hint was None

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).
    - [ ] 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 .

This PR needs a new test including amplitudes without magntiude_hint and with type and without type defined. It also needs a test with a wavefile with a full path given.

@calum-chamberlain calum-chamberlain added this to the 1.1.1 milestone Dec 5, 2017

@calum-chamberlain calum-chamberlain self-assigned this Dec 5, 2017

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Dec 5, 2017

Contributor

Tests added and pass locally. Awaiting CI and review.

Contributor

calum-chamberlain commented Dec 5, 2017

Tests added and pass locally. Awaiting CI and review.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Dec 6, 2017

Member

Do we have some devs using nordic to jump in for a review?

Member

megies commented Dec 6, 2017

Do we have some devs using nordic to jump in for a review?

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain
Contributor

calum-chamberlain commented Dec 6, 2017

@shicks-seismo

This comment has been minimized.

Show comment
Hide comment
@shicks-seismo

shicks-seismo Dec 6, 2017

Member

Yeah sure, do you mind if my review waits until after AGU?

Member

shicks-seismo commented Dec 6, 2017

Yeah sure, do you mind if my review waits until after AGU?

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Dec 6, 2017

Contributor

I'm fine with that, have a good AGU!

Contributor

calum-chamberlain commented Dec 6, 2017

I'm fine with that, have a good AGU!

@calum-chamberlain

This comment has been minimized.

Show comment
Hide comment
@calum-chamberlain

calum-chamberlain Jan 22, 2018

Contributor

@shicks-seismo are you able to give this a quick review sometime soon so that it can drop into 1.1.1?

Contributor

calum-chamberlain commented Jan 22, 2018

@shicks-seismo are you able to give this a quick review sometime soon so that it can drop into 1.1.1?

calum-chamberlain referenced this pull request in eqcorrscan/EQcorrscan Apr 6, 2018

Felix Halpaap
Fix needed for writing to NORDIC format
Merge remote-tracking branch 'origin/patch-1'

calum-chamberlain added some commits Dec 5, 2017

Bug-fix amplitude and wavefile write
- Wavefile lines were not correctly justified when full paths were given
- amplitudes were not handled when `magnitude_hint` was `None`
@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Apr 11, 2018

Member

I rebased on the latest version of the maintenance_1.1.x branch which should hopefully fix many of the CI errors.

Member

krischer commented Apr 11, 2018

I rebased on the latest version of the maintenance_1.1.x branch which should hopefully fix many of the CI errors.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Apr 11, 2018

Member

CI is green and the PR seems okay to me but I don't really know anything about nordic files so somebody else please take responsibility for this. Can this be merged?

Member

krischer commented Apr 11, 2018

CI is green and the PR seems okay to me but I don't really know anything about nordic files so somebody else please take responsibility for this. Can this be merged?

Show outdated Hide outdated CHANGELOG.txt
@@ -1109,7 +1109,8 @@ def nordpick(event):
else:
amp = None
coda = ' '
if amplitude.magnitude_hint.upper() == 'ML':
mag_hint = (amplitude.magnitude_hint or amplitude.type)
if mag_hint is not None and mag_hint.upper() in ['AML', 'ML']:
phase_hint = 'IAML'

This comment has been minimized.

@megies

megies Apr 11, 2018

Member

Why this is mapping 'ML' to 'IAML' in the first place?

@megies

megies Apr 11, 2018

Member

Why this is mapping 'ML' to 'IAML' in the first place?

This comment has been minimized.

@calum-chamberlain

calum-chamberlain Apr 19, 2018

Contributor

I've done this because that is how local magnitude picks are read in from Nordic (e.g. I've done this because I did this elsewhere) - When picking amplitudes for local magnitudes in seisan they are stored in the Nordic file as picks of type IAML.
If you are happy with this then I'm good to merge?

@calum-chamberlain

calum-chamberlain Apr 19, 2018

Contributor

I've done this because that is how local magnitude picks are read in from Nordic (e.g. I've done this because I did this elsewhere) - When picking amplitudes for local magnitudes in seisan they are stored in the Nordic file as picks of type IAML.
If you are happy with this then I'm good to merge?

This comment has been minimized.

@megies

megies Apr 20, 2018

Member

euhm.. I only realized now.. you're setting this on phase_hint even, which is supposed to hold a wave onset pick, not a magnitude type identification.. so is this misusing the Pick object to store the timing info of an amplitude measurement?

@megies

megies Apr 20, 2018

Member

euhm.. I only realized now.. you're setting this on phase_hint even, which is supposed to hold a wave onset pick, not a magnitude type identification.. so is this misusing the Pick object to store the timing info of an amplitude measurement?

This comment has been minimized.

@calum-chamberlain

calum-chamberlain Apr 20, 2018

Contributor

It is indeed putting the amplitude timing into a pick object. But this is only done internally I think? Prior to writing. I could rewrite it so that it has a separate, mostly duplicated way of writing out amplitude info (the lines in Nordic are mostly the same, everything is treated as a pick).

@calum-chamberlain

calum-chamberlain Apr 20, 2018

Contributor

It is indeed putting the amplitude timing into a pick object. But this is only done internally I think? Prior to writing. I could rewrite it so that it has a separate, mostly duplicated way of writing out amplitude info (the lines in Nordic are mostly the same, everything is treated as a pick).

This comment has been minimized.

@megies

megies Apr 23, 2018

Member

Ah well, maybe I'm just looking at the QuakeML standards a bit differently and it's OK that way. The phase_hint might be slightly misused, but hey if you guys actually using this format think it's fine this way, that's good enough for me.

@megies

megies Apr 23, 2018

Member

Ah well, maybe I'm just looking at the QuakeML standards a bit differently and it's OK that way. The phase_hint might be slightly misused, but hey if you guys actually using this format think it's fine this way, that's good enough for me.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Apr 11, 2018

Member

but I don't really know anything about nordic files

Same here, we can merge this after a nod by @d-chambers or @shicks-seismo I guess.

Member

megies commented Apr 11, 2018

but I don't really know anything about nordic files

Same here, we can merge this after a nod by @d-chambers or @shicks-seismo I guess.

@megies megies requested review from d-chambers and shicks-seismo Apr 11, 2018

@megies megies changed the title from Bug-fix amplitude and wavefile write to nordic: Bug-fix amplitude and wavefile write Apr 12, 2018

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Apr 20, 2018

Member

blah.. not sure why appveyor fails with some pip issue.. :-/
(Ah but the commit before it passed on appveyor and the last commit was just adding to the changelog)

Member

megies commented Apr 20, 2018

blah.. not sure why appveyor fails with some pip issue.. :-/
(Ah but the commit before it passed on appveyor and the last commit was just adding to the changelog)

@d-chambers

This comment has been minimized.

Show comment
Hide comment
@d-chambers

d-chambers Apr 20, 2018

Member

@megies looks like the same Appveyor issue we are seeing in #2091

Member

d-chambers commented Apr 20, 2018

@megies looks like the same Appveyor issue we are seeing in #2091

# Pick to associate with amplitude
test_event.picks.append(
# Pick to associate with amplitude - 0
test_event.picks = [

This comment has been minimized.

@d-chambers

d-chambers Apr 20, 2018

Member

Good changes, this makes the code more readable.

@d-chambers

d-chambers Apr 20, 2018

Member

Good changes, this makes the code more readable.

@d-chambers

Overall the code looks fine. There are some things about the nordic module/tests that could be made more concise but nothing that pertains directly to this PR.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Apr 23, 2018

Member

Alright, you guys seem happy with this one, so I'll merge, since the appveyor fail is unrelated and an earlier commit (after which no code changes were done anymore) passed on appveyor.

Member

megies commented Apr 23, 2018

Alright, you guys seem happy with this one, so I'll merge, since the appveyor fail is unrelated and an earlier commit (after which no code changes were done anymore) passed on appveyor.

@megies megies merged commit 3512c30 into maintenance_1.1.x Apr 23, 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

@megies megies deleted the patch-nordic-amps branch Apr 23, 2018

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