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

Fix gse2 sta mag amp #2420

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@thefroid
Copy link
Contributor

commented Jun 26, 2019

What does this PR do?

The plugin _read_gse2 was not able to read Mag2 for each station if a Mag1 was not defined.
I corrected it to read Mag2 for each station even if Mag1 is not defined.
Moreover, an amplitude object is not created if information about magnitude is not present,
more explicitly i don't fetch amplitude, period... if we don't know what type of amplitude is computed.

Why was it initiated? Any relevant Issues?

Please fill in

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 .
public_id = "magnitude/station/%s/%s" % (line_id, i)
sta_mag.resource_id = self._get_res_id(public_id)
event.station_magnitudes.append(sta_mag)
if flag_sta_mag:
event.amplitudes.append(amplitude)

This comment has been minimized.

Copy link
@megies

megies Jun 26, 2019

Member

Does it hurt to add the amplitude even if no station magnitude is using it?

@@ -2,7 +2,6 @@
# -*- coding: utf-8 -*-
"""
The gse2.bulletin test suite.

This comment has been minimized.

Copy link
@megies

megies Jun 26, 2019

Member

This blank line should stay in

@megies

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

Right now, any PR should be based against master.

@thefroid thefroid force-pushed the thefroid:fix_gse2_sta_mag_amp branch from b7bcdf8 to fe82f51 Jun 26, 2019

@thefroid

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

Ok it's not a problem, i added the amplitude even if there is no station magnitude.

@thefroid thefroid changed the base branch from maintenance_1.1.x to master Jun 26, 2019

@megies

This comment has been minimized.

Copy link
Member

commented Jun 26, 2019

2 pep8 fails, see circle CI log at bottom

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.