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

skip chans without instrument sensitivity #2200

Merged
merged 3 commits into from Oct 18, 2018

Conversation

Projects
None yet
4 participants
@dsentinel
Copy link
Contributor

dsentinel commented Jul 17, 2018

When writing SAC pz's to a file from inventory, if there channels that do not have a PolesZerosResponsStage (e.g. SOH), writing will fail raising and exception.

@dsentinel dsentinel force-pushed the write_sacpz_skip_sr0 branch from 134ee08 to b3bc918 Jul 24, 2018

@dsentinel

This comment has been minimized.

Copy link
Contributor Author

dsentinel commented Jul 24, 2018

Currently inv.write(f, format='SACPZ') with fail with exception.
Here is a gist of the current PR.

@megies

This comment has been minimized.

Copy link
Member

megies commented Jul 25, 2018

I wouldn't use that file format you're writing to in production workflows, but I guess this change can not hurt..

msg = "Encountered unrecognized input units in response: "
raise NotImplementedError(msg + str(input_unit))
print("{}.{}.{} ".format(net.code, sta.code, cha.code) +\
"has unrecognized input units in " +\

This comment has been minimized.

@megies

megies Jul 25, 2018

Member

please remove those redundant backslashes (see circle CI)

This comment has been minimized.

@dsentinel

dsentinel Jul 25, 2018

Author Contributor

That was ugly.

paz = resp.get_paz()
try:
paz = resp.get_paz()
except:

This comment has been minimized.

@megies

megies Jul 25, 2018

Member

Please only catch that particular exception that is being raised when no PAZ is found.

This comment has been minimized.

@dsentinel

dsentinel Jul 25, 2018

Author Contributor

I typically do, but it raises a base Exception.

raise Exception(msg)

This comment has been minimized.

@QuLogic

QuLogic Jul 25, 2018

Member

That's not the base, BaseException is. While Exception is not great, catching it is still preferable as it won't catch stuff like KeyboardInterrupt.

@megies
Copy link
Member

megies left a comment

Needs some minor changes, otherwise seems fine. Please also add a one-liner in changelog.

@megies megies added this to the 1.2.0 milestone Jul 25, 2018

@dsentinel

This comment has been minimized.

Copy link
Contributor Author

dsentinel commented Jul 25, 2018

I wouldn't use that file format you're writing to in production workflows, but I guess this change can not hurt..

You wouldn't use SAC PZ's as a file format? I wouldn't either! ;)
There are, unfortunately, still many sac users and when given a StationXML; Convert to dataless, convert to sacpz.
This is to help legacy users.

@krischer
Copy link
Member

krischer left a comment

Two small changes and please add one or two test cases triggering both cases. Otherwise I'm also fine with this :-)

except Exception:
print("{}.{}.{} has no paz. Skipping.".format(net.code,
sta.code,
cha.code))

This comment has been minimized.

@krischer

krischer Jul 26, 2018

Member

I would also print the epoch times of the channel to uniquely identify it. And instead of printing please raise warnings.

This comment has been minimized.

@megies

megies Jul 26, 2018

Member

Oh overlooked this.. yes definitely a warning instead.

msg += "has unrecognized input units in "
msg += "response: {}. Skipping".format(input_unit)
print(msg)
continue

This comment has been minimized.

@krischer

krischer Jul 26, 2018

Member

Same comment as above.

@krischer
Copy link
Member

krischer left a comment

I actually wanted to request changes previously 🙈

@dsentinel

This comment has been minimized.

Copy link
Contributor Author

dsentinel commented Jul 30, 2018

Can I include a test stationxml, like the one in the test case gist above? A quick look didn't show any in the repo with SOH channels.

@megies

This comment has been minimized.

Copy link
Member

megies commented Jul 31, 2018

Yes you can, but please limit it to what you really need in the test, it should be as small as possible (just a couple of channels, ...). Especially only include response if really needed for the tests as it is huge, usually.

@dsentinel

This comment has been minimized.

Copy link
Contributor Author

dsentinel commented Jul 31, 2018

I think everything's addressed.
I put a stationxml in obspy's stationxml tests/data. It has 2 channels ACE and QEP, it's 5kb.
Thanks guys!

@dsentinel

This comment has been minimized.

Copy link
Contributor Author

dsentinel commented Aug 1, 2018

Travis CI seems to be failing on 2 linux versions with:
The command "pip install https://github.com/megies/PyImgur/archive/py3.zip" failed and exited with 1 during .

Appveyor seems to have an unrelated failure:
FAIL: test_infinite_loop (obspy.io.mseed.tests.test_mseed_special_issues.MSEEDSpecialIssueTestCase)

self.assertEqual(len(w), 2)
inv.write(f, format='SACPZ')
# Only 2 newlines are written.
self.assertEqual(2, f.tell())

This comment has been minimized.

@megies

megies Aug 17, 2018

Member

What is wrong with testing the warning? How you were doing it was fragile though.. check out other parts of the code base that test for warning messages (should ideally be refactored in a utility routine at some point..)

+        # check emitted warning message
+        self.assertTrue(len(w) >= 1)
+        msg = ("'components' is expected to be a list of one or more "
+               "component code combinations (e.g. `components=['Z12']` or "
+               "`components=['Z12', '123']`)")
+        for w_ in w:
+            try:
+                self.assertEqual(str(w_.message), msg)
+            except AssertionError:
+                continue
+            else:
+                break
+        else:
+            self.fail(msg='Expected warning message not shown.')

This comment has been minimized.

@dsentinel

dsentinel Aug 22, 2018

Author Contributor

I found it depended on state, which could be alleviated with warnings.simplefilter('always', UserWarning) but this changes state for the future, and I just felt like I was going down a rabbit hole.
I can put something like this back in, but as this PR's purpose is to ignore chans instead of crashing, I went with the latter solution.

sta.code,
cha.location_code,
cha.code,
cha.start_date)

This comment has been minimized.

@megies

megies Aug 17, 2018

Member

no need to keep everything on a separate line..

sta.code,
cha.location_code,
cha.code,
cha.start_date)

This comment has been minimized.

@megies

megies Aug 17, 2018

Member

same a above..

@dsentinel

This comment has been minimized.

Copy link
Contributor Author

dsentinel commented Sep 6, 2018

Ready to close this if tests are acceptable.

@megies
Copy link
Member

megies left a comment

Warnings need to be dealt with, otherwise OK I think.

'data', 'only_soh.xml')
inv = read_inventory(path)
f = io.StringIO()
inv.write(f, format='SACPZ')

This comment has been minimized.

@megies

megies Sep 11, 2018

Member

This triggers two (expected) warnings. They need to be properly caught and tested/asserted. (Look for catch_warnings(...) in our test files)

@dsentinel dsentinel force-pushed the write_sacpz_skip_sr0 branch from f1f83e5 to b72dd23 Sep 13, 2018

@dsentinel

This comment has been minimized.

Copy link
Contributor Author

dsentinel commented Sep 14, 2018

Warnings caught.
Squashed and force pushed.
No rebase as master hasn't moved.

with warnings.catch_warnings(record=True) as w:
warnings.simplefilter('always')
inv.write(f, format='SACPZ')
self.assertEqual(len(w), 2)

This comment has been minimized.

@megies

megies Sep 17, 2018

Member

Ideally, this should also check the warning message, you might always get some warnings that are something else entirely.

I'm testing warnings like this nowadays:

for w_ in w:
try:
self.assertEqual(
str(w_.message),
'Encountered an event with origin uncertainty '
'description of type "confidence ellipsoid". This is '
'not yet implemented for output as shapefile. No '
'origin uncertainty will be added to shapefile for '
'such events.')
except AssertionError:
continue
break
else:
raise

Ideally we should put this in a helper routine in obspy/core/util/testing.py..

def assert_warning(warnings, message_regex, category=UserWarning):
    ...

This comment has been minimized.

@dsentinel

dsentinel Sep 17, 2018

Author Contributor

Py3 has assertWarns (which I originally implemented) and assertWarnsRegex, but not py2.
Although this may be remedied with the backport import unittest2, if we want to go that route.

I cringe at copy/pasting something from code to tests.
Catching exactly 2 warnings from the test file that has 2 warning conditions seemed sufficient.
I can assert they are UserWarnings, or they end in '. Skipping.' or a full regex.
Anyway, thanks for caring about the code base. I'll proceed as advised.

@dsentinel dsentinel force-pushed the write_sacpz_skip_sr0 branch from b72dd23 to 559842a Oct 10, 2018

@dsentinel dsentinel force-pushed the write_sacpz_skip_sr0 branch from 559842a to d4b27e9 Oct 10, 2018

@dsentinel

This comment has been minimized.

Copy link
Contributor Author

dsentinel commented Oct 11, 2018

Warning messages dealt.

@dsentinel

This comment has been minimized.

Copy link
Contributor Author

dsentinel commented Oct 17, 2018

Merge-able?

@megies megies merged commit 40dfdcc into master Oct 18, 2018

1 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details

@megies megies deleted the write_sacpz_skip_sr0 branch Oct 18, 2018

@dsentinel

This comment has been minimized.

Copy link
Contributor Author

dsentinel commented Nov 5, 2018

Thanks @megies @krischer @QuLogic !!

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.