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

mseed.utils.get_start_and_end_time returning sr=1 when sr=0. #2069

Merged
merged 8 commits into from Feb 27, 2018

Conversation

Projects
None yet
3 participants
@dsentinel
Contributor

dsentinel commented Feb 7, 2018

get_start_and_end_time() returns sr of 1 when sr is 0

Another little piece that doesn't handle sr=0 channels such LOGs correctly

@dsentinel dsentinel changed the title from Fix returning sr=1 when sr=0. to mseed.utils.get_start_and_end_time returning sr=1 when sr=0. Feb 7, 2018

@krischer

This comment has been minimized.

Member

krischer commented Feb 8, 2018

Thanks for this. I think it makes sense to handle it the way you coded it. Can you please also add a test as well as a changelog entry? A simple way to test this is to just use one of the existing test files, load it to a io.BytesIO object and replace the two bytes in question with zeros and then read the modified object - that way no new test files would be required.

@megies megies added this to the 1.1.1 milestone Feb 8, 2018

@megies megies added the .io.mseed label Feb 8, 2018

@dsentinel

This comment has been minimized.

Contributor

dsentinel commented Feb 8, 2018

Thanks for this. I think it makes sense to handle it the way you coded it. Can you please also add a test as well as a changelog entry?

Happy to.

I have a LOG file that tripped this up (ASCII LOG from an RT-130) . It's 61Kb. I can add it, or do as you suggested, or cut it down to a few blocks. A quick scan didn't reveal any samplerate=0 mseed in test data. May be worth having one?

@krischer

This comment has been minimized.

Member

krischer commented Feb 9, 2018

Please cut it down to one or two records if you choose to add. Personally I would prefer if you just generate the required test data on the fly but I'm okay with small test files as well if you prefer it.

dsentinel added some commits Feb 12, 2018

@dsentinel

This comment has been minimized.

Contributor

dsentinel commented Feb 12, 2018

Should be ready to merge.

@krischer

Nice test!

Please make sure that the code formatting tests pass: https://circleci.com/gh/obspy/obspy/1383?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

Locally you can just install flake8 and do

$ flake8 obspy/io/mseed/tests/test_mseed_util.py obspy/io/mseed/util.py
@@ -12,6 +12,7 @@
- obspy.io.mseed:
* Ability to read files that have embedded chunks of non SEED data.
(see #1981, #2057).
* Fix util.get_start_and_end_time returning sr=0 when sr=1 (see #2069)

This comment has been minimized.

@krischer

krischer Feb 13, 2018

Member

Can you write out sampling rate to make it easier to understand?

@dsentinel

This comment has been minimized.

Contributor

dsentinel commented Feb 13, 2018

Cheers.
Of course flake bit me. :)

@megies

This comment has been minimized.

Member

megies commented Feb 15, 2018

You can also see #2076 for a real world test file.

@dsentinel

This comment has been minimized.

Contributor

dsentinel commented Feb 19, 2018

Thanks @megies! I'll give it a go.
Are you suggesting that we include it in tests? If so I can add it as a test case, and #2076 can use as well.

@krischer

This comment has been minimized.

Member

krischer commented Feb 21, 2018

Are you suggesting that we include it in tests? If so I can add it as a test case, and #2076 can use as well.

Sounds like a good plan. Especially if we have a couple more bugs related to traces with a sampling rate of 0.

@dsentinel

This comment has been minimized.

Contributor

dsentinel commented Feb 22, 2018

I added the LOG file to tests/data, and a generic reading test for it as well as a test case for this PR.
There was also #1990 related to sr=0, but this was limited to stats.

@dsentinel

This comment has been minimized.

Contributor

dsentinel commented Feb 22, 2018

Should be ready to merge if tests pass.

@megies

megies approved these changes Feb 26, 2018

Looks good to me, but I'll leave this to @krischer to merge

@krischer

This comment has been minimized.

Member

krischer commented Feb 27, 2018

Looks good to me. Thanks a bunch!

@krischer krischer merged commit abe900d into maintenance_1.1.x Feb 27, 2018

3 of 4 checks passed

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

@krischer krischer deleted the fix_get_start_and_end_time_of_0hz branch Feb 27, 2018

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