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

Skipping empty and incomplete StationXML channels #1840

Merged
merged 4 commits into from Jul 21, 2017

Conversation

Projects
None yet
3 participants
@krischer
Member

krischer commented Jul 12, 2017

Fixes #1839.

krischer added some commits Jul 12, 2017

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Jul 12, 2017

Member

Completely empty channels are silently skipped, channels with unset coordinates are also skipped but a warning is raised.

Member

krischer commented Jul 12, 2017

Completely empty channels are silently skipped, channels with unset coordinates are also skipped but a warning is raised.

@megies

megies approved these changes Jul 12, 2017

Changes look good to me.. let's see if CI agrees.

@Jollyfant

This comment has been minimized.

Show comment
Hide comment
@Jollyfant

Jollyfant Jul 12, 2017

Contributor

I would hoist the raised message to the _read_station level and put the station name in the user warning.

if cha is not None:
    channels.append(cha)
else:
    warnings.warn(..., UserWarning)

Right now it says 00.BHZ is empty but.... which station!?

Contributor

Jollyfant commented Jul 12, 2017

I would hoist the raised message to the _read_station level and put the station name in the user warning.

if cha is not None:
    channels.append(cha)
else:
    warnings.warn(..., UserWarning)

Right now it says 00.BHZ is empty but.... which station!?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Jul 13, 2017

Member

I would hoist the raised message to the _read_station level

👍 We could raise a custom exception on channel level that gets caught on station level to show the warning..

Member

megies commented Jul 13, 2017

I would hoist the raised message to the _read_station level

👍 We could raise a custom exception on channel level that gets caught on station level to show the warning..

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Jul 21, 2017

Member

@Jollyfant Done. Sorry for the delay.

The internal _read_channel() method will now return None if, and only if, one of the coordinates is not set. That triggers the warning to be raised by the _read_station() method. This is not as clean as raising and catching a custom exception but that seemed a bit over-engineering to be me and it all happens only deep down in some internal functions.

Member

krischer commented Jul 21, 2017

@Jollyfant Done. Sorry for the delay.

The internal _read_channel() method will now return None if, and only if, one of the coordinates is not set. That triggers the warning to be raised by the _read_station() method. This is not as clean as raising and catching a custom exception but that seemed a bit over-engineering to be me and it all happens only deep down in some internal functions.

@Jollyfant

This comment has been minimized.

Show comment
Hide comment
@Jollyfant

Jollyfant Jul 21, 2017

Contributor

I'm happy feel free to merge 🐻

Contributor

Jollyfant commented Jul 21, 2017

I'm happy feel free to merge 🐻

@krischer krischer merged commit 9694d5f into master Jul 21, 2017

5 of 6 checks passed

docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 90%)
Details
codecov/project 87.73% (+1.44%) compared to 5b057eb
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@krischer krischer deleted the stationxml-empty-channel branch Jul 21, 2017

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