Added support for output SAC headers with geocentric coordinate system

krischer / instaseis

Instant high-frequency seismograms from an AxiSEM database

http://instaseis.net

Other

51 stars 23 forks source link

Added support for output SAC headers with geocentric coordinate system #80

Closed manoch-iris closed 3 years ago

manoch-iris commented 3 years ago

Instaseis now accepts "sacheader" parameter with the default value of "geodetic" and optional value of "geocentric" to control how an output SAC file header is populated. If the sacheader parameter is not provided or is set to "geodetic", there will be no change in the output SAC headers. However, if the parameter is set to "geocentric", the output SAC files header will be populated using geocentric coordinates. This option is intended as a support for the Mars models.

coveralls commented 3 years ago

Coverage increased (+0.0005%) to 99.928% when pulling caeb8e4185704441061fca221c3cfd2a9137a546 on manoch-iris:origin/sacheader into a2b88a09845291d1a8f131a88128988c3e32eebe on krischer:master.

manoch-iris commented 3 years ago

Hello,

I do not understand why this pull request is failing. It flags lines 2574 and 2575 of test_server.py but I do not see why. Additional information will be highly appreciated.

Thank you, --manoch

martinvandriel commented 3 years ago

Hi Manoch,

I think this is just coverage: you either need to add a test that hits these lines, or you can flag them as not required to be tested by appending a # pragma: no cover.

Cheers, Martin

manoch-iris commented 3 years ago

Hi Martin,

Thank you for your reply. It only flags my except and pass lines below. During my test I do hit this try block. Is there any particular reason why except and pass lines are flagged?

Thanks, --manoch

   try:
        del tr.stats.sac
    **except KeyError:
        pass**

martinvandriel commented 3 years ago

Hi Manoch,

I would assume this means the KeyError is never raised in the tests.

Martin

manoch-iris commented 3 years ago

Thank you Martin! I will check and resubmit my pull request.

--manoch

chad-earthscope commented 3 years ago

@krischer @martinvandriel Do you have any estimate (or wild guess) at when this may be reviewed/merged? We are considering releasing an update to Syngine to include Mars synthetics with a kludge for the coordinate transformation if this will be a while.

krischer commented 3 years ago

Sorry for the delay here. My github inbox is a bit of a mess these days.

I pushed one more commit to adapt the formatting to the rest of the code base and I'm happy to merge this now.

In general I wonder if this should rather be a configuration parameter of the server or the database but this is such a niche use case and I guess you are the only ones who'll actually use. So if you are fine with this so am I.

Please note that the ellipticity correction will still be applied when parsing from QuakeML or StationXML files but I also guess this does not matter for you.

Do you need a release to be able to use it?

chad-earthscope commented 3 years ago

Thanks @krischer!

Do you need a release to be able to use it?

IRIS needs ETHZ to upgrade their Mars Instaseis services, so whatever they need is our need. @martinvandriel ?

IRIS does not need a release to install it; we don't even really need to upgrade our servers (which are only Earth models), but can if more testing is needed.

martinvandriel commented 3 years ago

I sent out an email to everybody involved.