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

SAC reference time not automatically set for incomplete stats.sac header #1285

Merged
merged 14 commits into from Mar 10, 2016

Conversation

Projects
None yet
5 participants
@jkmacc-LANL
Copy link
Contributor

jkmacc-LANL commented Feb 26, 2016

I want to set the 'o' sac header in a newly created Trace. This used to work with obspy<1.0 which automatically set the reference time headers. Now reference time is only set if no sac stats entry is present.

In [1]: tr = read()[0]
In [2]: from obspy.io.sac import SACTrace
In [3]: tr.stats.sac = {'o': 0}
In [5]: sactr = SACTrace.from_obspy_trace(tr)
/home/richter/anaconda/lib/python2.7/site-packages/obspy/io/sac/util.py:333: UserWarning: Old header has invalid reftime.
  warnings.warn(msg)
In [6]: print sactr
/home/richter/anaconda/lib/python2.7/site-packages/obspy/io/sac/sactrace.py:1395: UserWarning: Reference time information incomplete.
  warnings.warn(msg)
Reference Time = XX/XX/XX (XXX) XX:XX:XX.XXXXXX
    iztype not set
delta      = 0.00999999977648
iftype     = itime
kcmpnm     = EHZ
knetwk     = BW
kstnm      = RJOB
lcalda     = True
leven      = True
lovrok     = True
lpspol     = False
npts       = 3000
nvhdr      = 6
o          = 0.0

I help myself with

In [8]: from obspy.io.sac.util import obspy_to_sac_header
In [9]: tr = read()[0]
In [10]: tr.stats.sac = obspy_to_sac_header(tr.stats)
In [14]: tr.stats.sac['o'] = 0
In [15]: sactr = SACTrace.from_obspy_trace(tr)
In [16]: print sactr
Reference Time = 08/24/2009 (236) 00:20:03.000000
    iztype IB: begin time
b          = 0.0
delta      = 0.00999999977648
e          = 29.9899993297
iftype     = itime
iztype     = ib
kcmpnm     = EHZ
knetwk     = BW
kstnm      = RJOB
lcalda     = True
leven      = True
lovrok     = True
lpspol     = False
npts       = 3000
nvhdr      = 6
nzhour     = 0
nzjday     = 236
nzmin      = 20
nzmsec     = 0
nzsec      = 3
nzyear     = 2009
o          = 0.0
scale      = 1.0

Maybe this edge case could be handled inside io.sac?

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Feb 23, 2016

Could you just switch [3] and [5]?

In [1]: from obspy.io.sac import SACTrace

In [2]: from obspy import read

In [3]: tr = read()[0]

In [4]: sactr = SACTrace.from_obspy_trace(tr)

In [5]: sactr.o = 0

In [6]: print sactr
Reference Time = 08/24/2009 (236) 00:20:03.000000
    iztype IB: begin time
b          = 0.0
delta      = 0.00999999977648
e          = 29.9899993297
iftype     = itime
iztype     = ib
kcmpnm     = EHZ
knetwk     = BW
kstnm      = RJOB
lcalda     = True
leven      = True
lovrok     = True
lpspol     = False
npts       = 3000
nvhdr      = 6
nzhour     = 0
nzjday     = 236
nzmin      = 20
nzmsec     = 0
nzsec      = 3
nzyear     = 2009
o          = 0.0
scale      = 1.0

The problem with setting only oin a SAC header is that it produces an invalid SAC header (the origin time is 0 seconds relative to a reference time that doesn't exist), and it is hard to merge an ObsPy header and an invalid SAC header, especially because you have to start making assumptions about the intent of the user. When you make a SACTrace from just a Trace header, that set of logic is more straightforward. Setting o afterwards is just setting o. I'll look into what it would take to produce your expected behavior, but my preferred solution would be to discourage the creation of an invalid SAC header assuming that the rest of the required information is in the ObsPy Trace header.

Sorry for the over-explanation; I'm just trying to describe the reasoning behind the behavior you're seeing.

@QuLogic QuLogic added the .io.sac label Feb 23, 2016

@trichter

This comment has been minimized.

Copy link
Member Author

trichter commented Feb 24, 2016

I get you point and I think you are right in discouraging invalid stats.sac headers and reduce guessing of user intents. Still, there is some really strange behavior, e.g.:

In [1]: from obspy import read
In [2]: tr = read()[0]
In [3]: tr.stats.sac = {'o': 0}
In [5]: tr.write('TEST.SAC', 'SAC')
/home/richter/anaconda/lib/python2.7/site-packages/obspy/io/sac/util.py:333: UserWarning: Old header has invalid reftime.
  warnings.warn(msg)
In [6]: tr2 = read('TEST.SAC')
In [7]: print tr2
1 Trace(s) in Stream:
BW.RJOB..EHZ | 1970-01-01T00:00:00.000000Z - 1970-01-01T00:00:29.990000Z | 100.0 Hz, 3000 samples

The start time of the trace is set to 1970.
Therefore, it could be better to be rigorous and raise an exception instead of showing a warning?

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Feb 24, 2016

I also just ran into exactly this issue with one of my codes. I agree with @trichter: Please raise an exception at the time of writing as this is not what a user would expect and it could cause weird and hard to debug issues later down the road.

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Feb 24, 2016

Ugh, that is nasty. Agreed, this behavior needs to go. There are 21 io.sac tests that fail if a valid SAC header is required upon write, though, and 3 tests that fail if we require only valid reference time headers (the test SAC ascii/xy file has unset reference time headers). The test suite freezes in some funky behavior, unfortunately, much of it relating to this header merging issue.

Because header merging seems to be the expected behavior, it may be less disruptive to try to fix these types of issues as they come up with better header-merging code, rather than to abruptly start requiring valid SAC files on write, at least for the time being. @trichter I'll try to produce your expected merged header rather than raise an exception. There's always the option of better validating the file before writing using SACTrace.validate.

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Feb 24, 2016

We really have to look into this a bit more and fairly soon but I think we do need some auto-header merging. Setting any sac header field, i.e. not even time related ones will reset the time of the trace which is not expected and will break existing codes.

In [1]: import obspy

In [2]: tr = obspy.read()[0]

In [3]: tr.stats.sac = {"stla": 12}

In [4]: tr.write("blub.sac", format="sac")
/Users/lion/workspace/code/obspy/obspy/io/sac/util.py:333: UserWarning: Old header has invalid reftime.
  warnings.warn(msg)

In [5]: print(obspy.read("blub.sac"))
1 Trace(s) in Stream:
BW.RJOB..EHZ | 1970-01-01T00:00:00.000000Z - 1970-01-01T00:00:29.990000Z | 100.0 Hz, 3000 samples

I'm not sure what's the best way to do this but I think it has to happen. Any suggestions?

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Feb 24, 2016

Changes would go into io.sac.util:obspy_to_sac_header. I'll just need to hack on it, I think. Some of the needed changes should be obvious, but it's not always obvious how any changes play with the SAC test suite. That was the wildcard during development, anyway. I'll try to do this quickly.

@trichter

This comment has been minimized.

Copy link
Member Author

trichter commented Feb 26, 2016

There is another option open for discussion.

  • Refactor most parts of SACTrace to a SACHeader class only dealing with the headers
  • The parts of SACTrace dealing with data stay in the SACTrace class or could be refactored to functions
  • SACHeader could be directly attached to Trace.stats.sac upon reading to put all those SAC header magic directly in the Trace object (e.g. setting the field 'o' to a UTCDateTime instance)
  • In the setter method for stats.sac any input could be directly converted to a SACHeader instance or users are encouraged to do tr.stats.sac = SACHeader(). As a fall-back, the header has to be converted to SACHeader upon writing. (Here is the part to deal with the header merging problem.)

But well, it would introduce another API change 1.0.0 -> 1.0.1 which probably is not an option? Was just an idea...

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Feb 26, 2016

Arg, testxy.sac again 😠

Not sure why the stla=35.4 gets changed to a 35.400002 here.

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Feb 29, 2016

@trichter @krischer I've fixed the behavior you're seeing. Now, simple header merging behaves as expected. You'll see there are still some problems with the tests, though, particularly those using testxy.sac, which is an invalid SAC file. It's hard to read it and write it while keeping it invalid 😄 (I'm not sure we should).

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Mar 2, 2016

Anyone know what's happening here? Other than the mysterious flake8 failure, I'm happy with this PR.

self._hf[HD.FLOATHDRS.index('baz')] = baz
self._hf[HD.FLOATHDRS.index('dist')] = dist
self._hf[HD.FLOATHDRS.index('gcarc')] = gcarc
except (ValueError, TypeError) as e:

This comment has been minimized.

Copy link
@krischer

krischer Mar 2, 2016

Member

The as e triggers the flake8 warning as its not used anywhere.

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Mar 2, 2016

Looks mostly good to me. Can you fix the flake8 issue. I'll review it in more detail tomorrow.

@megies

This comment has been minimized.

Copy link
Member

megies commented Mar 3, 2016

Anyone know what's happening here? Other than the mysterious flake8 failure, I'm happy with this PR.

We're getting more errors from a newer pep8/flake8, I've already seen this locally on my machine after an update. I guess we either have to..

  • restrict the pep8/flake8 version numbers in CI (not ideal)
  • add those new error codes to the ignored ones (sub-optimal)
  • actually handle those warnings (best)

This is something that should be done on master soonish and then you can rebase this branch here.

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Mar 3, 2016

We definitely have to fix those in the master.

For future releases I think we should actually disable all the code formatting tests as otherwise tests will fail with new flake8 versions - I'll open an issue.

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Mar 3, 2016

OK, I think I've flake8-fixed the files under io/sac and all I intended with this PR.

@megies megies added this to the 1.0.x milestone Mar 3, 2016

@megies megies force-pushed the sac_obspy_header_merge branch from 6982491 to f7a12ce Mar 3, 2016

@megies

This comment has been minimized.

Copy link
Member

megies commented Mar 3, 2016

@jkmacc-LANL, I did a rebase and force-push. This should fix the left-over CI fails. If you need to do further work here remember to get the local branch in line with the new history.

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Mar 3, 2016

If you need to do further work here remember to get the local branch in line with the new history.

Thanks, Tobias. I think I'm done. I have a tiny docstring tweak, but I'll do it on a new PR (with new history), as it's completely unrelated.

@megies megies force-pushed the sac_obspy_header_merge branch from f7a12ce to 41482ab Mar 3, 2016

@megies

This comment has been minimized.

Copy link
Member

megies commented Mar 3, 2016

My bad, rebased on my local branch not the upstream updated one. Doh it again.

@megies

This comment has been minimized.

Copy link
Member

megies commented Mar 4, 2016

Uhm.. did I do something wrong during rebasing? There's some CI fails that I thought were fixed already.. @jkmacc-LANL, can you do a git log (and maybe a git diff maintenance_1.0.xon your local branch, assuming it's still on the state before my rebase?

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Mar 4, 2016

I haven't done anything with my local sac_obspy_header_merge in relation to your rebase. My git log looks like:

commit 698249130dacf2e6d4b35338f1d8204079d5b1d4
Author: Jonathan MacCarthy <jkmacc@lanl.gov>
Date:   Thu Mar 3 07:58:31 2016 -0700

    Update obspy_to_sac_header docstring

commit 7d356d0554ba7f019f802ebdbb7172fcae4b9f8f
Author: Jonathan MacCarthy <jkmacc@lanl.gov>
Date:   Thu Mar 3 07:57:50 2016 -0700

    Flake8

commit 110671a2562a265bc4667cb1f6dc199ed234af7b
Author: Jonathan MacCarthy <jkmacc@lanl.gov>
Date:   Thu Mar 3 07:03:05 2016 -0700

    Fix bad variable name

commit ae045db8216a4d6d33aa219272a999ebe11edcf2
Author: Jonathan MacCarthy <jkmacc@lanl.gov>
Date:   Wed Mar 2 14:49:33 2016 -0700

    Flake8

commit debab577cae6017f46d24d3be43a7a0035058ef3
Merge: f7a3de2 e961baa
Author: Jonathan MacCarthy <jkmacc@lanl.gov>
Date:   Wed Mar 2 12:46:29 2016 -0700

    Merge branch 'sac_obspy_header_merge' of https://github.com/obspy/obspy into sac_obspy_header_merge

    Amend commit message.

commit f7a3de2c88e718630076889a7af4b908fee6e002
Author: Jonathan MacCarthy <jkmacc@lanl.gov>
Date:   Wed Mar 2 12:43:12 2016 -0700

    Make SACTrace.lcalda tolerant to missing values

    Fixes #1300 .

I do see differences between what I've got and what is here. When I git fetch --all and git diff upstream/sac_obspy_header_merge obspy/io/sac/util.py, there's quite a bit that has changed. I'm not sure what to do from here. push --force?

@megies

This comment has been minimized.

Copy link
Member

megies commented Mar 5, 2016

@jkmacc-LANL, I'm a bit at a loss, but I think I might really have botched the rebase.. if you still have your local branch before my rebase, can you please git push --force?

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Mar 5, 2016

Will do...done.

@jkmacc-LANL jkmacc-LANL closed this Mar 7, 2016

@jkmacc-LANL jkmacc-LANL reopened this Mar 7, 2016

@jkmacc-LANL jkmacc-LANL force-pushed the sac_obspy_header_merge branch from 71b1f74 to 6982491 Mar 7, 2016

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Mar 7, 2016

I suspect I could resolve these failures with the correct git rebase command. @megies could you enlighten me? 😜

@megies

This comment has been minimized.

Copy link
Member

megies commented Mar 8, 2016

I suspect I could resolve these failures with the correct git rebase command. @megies could you enlighten me? 😜

You can force-push if you want, I have no problem with that at all. Afterwards, rebasing is just a plain git rebase maintenance_1.0.x -- if you don't get merge conflicts, that's it -- if you do get merge conflicts though you might have to do some manual editing of files in between. You can leave the rebase to us if you feel unsure (although the possibility to abort in the middle of a rebase with git rebase --abort usually works pretty well).

jkmacc-LANL added some commits Feb 26, 2016

Allow unset iztype, remove reftime warning
Allow an unset iztype if there are no relative time headers set.  In
this case, warning for an unset reftime is not necessary because we can
reconstruct the reftime from the trace.stats.starttime.
Don't change SAC files with no "b" or reftime
Related to issue 1285, and test file "testxy.sac".  If there are relative time
headers, even if there's a "b", throw an exception. If there's a "b" and no
relative times, use the "b" and write the "e" and nothing else. If there's no
"b" and no relative times, write the SAC file like it's an "ib" type file and
proceed.
Add SACTrace test_lcalda
Tests that lcalda works for complete geographic values, is tolerant to
missing geographic values by default, and raises SacHeaderError when
setting geographic distances is forced but some values are missing.

@jkmacc-LANL jkmacc-LANL force-pushed the sac_obspy_header_merge branch from 6982491 to d8f5ab6 Mar 8, 2016

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Mar 8, 2016

I think it worked 😅 I'm happy with this.

@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Mar 9, 2016

Erm...it was good to go. Should I do something so that this can be merged?

@megies

This comment has been minimized.

Copy link
Member

megies commented Mar 10, 2016

I'll merge it on command line probably just the usual trivial changelog merge fail, thanks for the fix @jkmacc-LANL.

@megies megies merged commit d8f5ab6 into maintenance_1.0.x Mar 10, 2016

3 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jkmacc-LANL

This comment has been minimized.

Copy link
Contributor

jkmacc-LANL commented Mar 10, 2016

The least I can do. Thanks for the merge.

@megies megies deleted the sac_obspy_header_merge branch Mar 11, 2016

@QuLogic QuLogic modified the milestones: 1.0.x, 1.0.1 Mar 23, 2016

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.