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

Add NNSA_KB_Core read support #1332

Merged
merged 2 commits into from Apr 8, 2016

Conversation

Projects
None yet
4 participants
@atwinkelman
Contributor

atwinkelman commented Mar 14, 2016

See #1301

I believe I've included everything (test and even documentation) for NNSA_KB_Core reading support alongside CSS reading.

Let me know if something needs adjusting. I still feel new to git so there's lots to learn!

@megies

This comment has been minimized.

Member

megies commented Mar 15, 2016

It looks like you based this PR on the current master correctly, but replaced some files with copies of your earlier PR so that some changes in those files are lost. I'll make separate notes on each of these problems that need fixing.

Also, it seems there are some changes of file permissions in there (making that file executable)? E.g... :

screenshot from 2016-03-15 10 38 15

screenshot from 2016-03-15 10 42 03

I think these executable flags should also be removed again.

arguments to not download certain pieces of data. (see #1305)
* The mass downloader can now download stations that are part of a given
inventory object.

This comment has been minimized.

@megies

megies Mar 15, 2016

Member

Please restore these lines.

* Some bug fixes for the mass downloader. (see #1293, #1304)
* Some bug fixes for the mass downloader. (see #1293, #1304)
- obspy.css
* Read support for NNSA KB Core format waveform data. (see #1290, #1301)

This comment has been minimized.

@megies

megies Mar 15, 2016

Member

Please move your changelog entry up to the master: section.

@@ -49,8 +49,8 @@
# default order of automatic format detection
WAVEFORM_PREFERRED_ORDER = ['MSEED', 'SAC', 'GSE2', 'SEISAN', 'SACXY', 'GSE1',
'Q', 'SH_ASC', 'SLIST', 'TSPAIR', 'Y', 'PICKLE',
'SEGY', 'SU', 'SEG2', 'WAV', 'DATAMARK', 'CSS',
'AH', 'PDAS', 'KINEMETRICS_EVT']
'SEGY', 'SU', 'SEG2', 'WAV', 'DATAMARK', 'CSS',

This comment has been minimized.

@megies

megies Mar 15, 2016

Member

The space at the end will fail our flake8 tests..

@@ -7,11 +7,14 @@
from future.builtins import * # NOQA
import os
import sys

This comment has been minimized.

@megies

megies Mar 15, 2016

Member

This comes from an old copy of this file and was removed in master. Please remove.

import numpy as np
from obspy import Stream, Trace, UTCDateTime
from obspy.core.compatibility import from_buffer
from obspy.core.util.deprecation_helpers import \
DynamicAttributeImportRerouteModule

This comment has been minimized.

@megies

megies Mar 15, 2016

Member

This comes from an old copy of this file and was removed in master. Please remove.

@@ -41,7 +44,6 @@
def _is_css(filename):
"""
Checks whether a file is CSS waveform data (header) or not.

This comment has been minimized.

@megies

megies Mar 15, 2016

Member

Please leave the empty line in, for separation.

.. warning::
This function should NOT be called directly, it registers via the
ObsPy :func:`~obspy.core.stream.read` function, call this instead.
:type filename: str

This comment has been minimized.

@megies

megies Mar 15, 2016

Member

Please leave these blank lines in.

.. warning::
This function should NOT be called directly, it registers via the
ObsPy :func:`~obspy.core.stream.read` function, call this instead.
:type filename: str

This comment has been minimized.

@megies

megies Mar 15, 2016

Member

Please add blank lines after first line and before parameter descriptions, to be in line with the rest of our codebase.

import_map={},
function_map={
"isCSS": "obspy.io.css.core._is_css",
"readCSS": "obspy.io.css.core._read_css"})

This comment has been minimized.

@megies

megies Mar 15, 2016

Member

This whole block comes from an old copy of this file and should be removed again, it's not in current master anymore.

@megies

This comment has been minimized.

Member

megies commented Mar 15, 2016

Looks pretty good overall, mostly just some glitches from working on older versions of some files.
Please remove executable flags from these two files again. Also, feel free to add yourself to CONTRIBUTORS.txt.

@megies megies added this to the 1.1.0 milestone Mar 15, 2016

@atwinkelman

This comment has been minimized.

Contributor

atwinkelman commented Mar 22, 2016

I fixed the aforementioned issues. Let me know if there is anything else that needs to happen.

1.0.x:
- obspy.clients.fdsn:
* Local URLs are now recognized as valid URLs. (see #1309)
* Some bug fixes for the mass downloader. (see #1293, #1304)
* Some bug fixes for the mass downloader. (see #1293, #1304)

This comment has been minimized.

@megies

megies Mar 23, 2016

Member

Please fix the changed indent here.

def _read_css(filename, **kwargs):
"""
Reads a CSS waveform file and returns a Stream object.

This comment has been minimized.

@megies

megies Mar 23, 2016

Member

Please leave in this blank line.

@megies

This comment has been minimized.

Member

megies commented Mar 23, 2016

@megies

This comment has been minimized.

Member

megies commented Mar 31, 2016

Can you rebase on current master? This might get rid of the remaining fails..

@@ -27,6 +28,7 @@ def setUp(self):
# directory where the test files are located
self.path = os.path.join(os.path.dirname(__file__), 'data')
self.filename = os.path.join(self.path, 'test.wfdisc')

This comment has been minimized.

@QuLogic

QuLogic Mar 31, 2016

Member

Since you suffixed st_result, why not change these too instead of a generic 2? Though I prefer a prefix in this case.

This comment has been minimized.

@atwinkelman

atwinkelman Apr 5, 2016

Contributor

Good call, I've appended the self.filename variables and the test.wfdisc filenames.

@codecov-io

This comment has been minimized.

codecov-io commented Apr 5, 2016

Current coverage is 86.66%

Merging #1332 into master will increase coverage by +0.10% as of 712a057

@@            master   #1332   diff @@
======================================
  Files          369     367     -2
  Stmts        49841   49786    -55
  Branches         0       0       
  Methods          0       0       
======================================
  Hit          43145   43145       
  Partial          0       0       
+ Missed        6696    6641    -55

Review entire Coverage Diff as of 712a057

Powered by Codecov. Updated on successful CI builds.

@atwinkelman

This comment has been minimized.

Contributor

atwinkelman commented Apr 5, 2016

I've rebased to the current master (and squashed my various commits to a single one). Still one failed check though.

# check every line
for line in lines:
assert(len(line.rstrip(b"\n\r")) == 287)
assert(line[27] == b".")

This comment has been minimized.

@QuLogic

QuLogic Apr 5, 2016

Member

Use 27:28; b'123'[0] == 49, but b'123'[0:1] == b'1'.

assert(len(line.rstrip(b"\n\r")) == 287)
assert(line[27] == b".")
UTCDateTime(float(line[16:33]))
assert(line[73] == b".")

This comment has been minimized.

@QuLogic

QuLogic Apr 5, 2016

Member

[73] -> [73:74]

added NNSA_KB_CORE read support
corrected NNSA_KB_CORE read support

Update CONTRIBUTORS.txt

Corrected NNSA_KB_CORE tests

Renamed css/nnsa test files

corrections to obspy/io/css/core.py
@atwinkelman

This comment has been minimized.

Contributor

atwinkelman commented Apr 5, 2016

Is there a way to reset the appveyor check? It seems to have had an issue.

@atwinkelman

This comment has been minimized.

Contributor

atwinkelman commented Apr 6, 2016

I believe all tests pass. Let me know what's next.

@megies

This comment has been minimized.

Member

megies commented Apr 7, 2016

Please also remove the executable flags from setup.py, and changelog is mentioning the closed predecessor PR. Otherwise looking good and ready to merge to me. 👍

@atwinkelman

This comment has been minimized.

Contributor

atwinkelman commented Apr 8, 2016

Should be all wrapped up now. Thanks for your patience!

@megies megies merged commit 9a12ab3 into obspy:master Apr 8, 2016

5 checks passed

codecov/patch 97.00% of diff hit (target 1.00%)
Details
codecov/project 86.66% (+0.10%) compared to d006bc9 at 86.56%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.005%) to 86.209%
Details
@megies

This comment has been minimized.

Member

megies commented Apr 8, 2016

Thanks for the contribution, good work there!

Btw.. now that this is merged you should ideally think about a git reset --hard for your master branch (stash any local changes first if you want to keep them!). It should really have the same history as our main master, because right now you couldn't make a valid PR from your master anymore (as it isn't based on the same history as our master).
(In general it's also not a bad idea to make obspy/obspy master the remote tracking branch for your local master.)

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