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
SC3ML: Set depth to 0 if missing, fix issue with finding publicIDs (fixes #1816) #1817
Conversation
Thanks for the quick action! Someone should remember to write to the mailing list once this is merged. |
👍 Great, this fixes a similar issue I had with reading SC3ML. I thought the XML was not valid and inserted the missing depth manually into the XML file. |
@krischer I tried mailing to the list but it wouldn't let me do it. Said I needed to sub first. I pointed Steve who reported this to the thread in a private mail instead. I'll add a to-do for it anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks for working on this, I also had a quick look and would propose some minor adaptions.
Please also add short changelog entry.
obspy/io/seiscomp/sc3ml.py
Outdated
@@ -383,6 +383,9 @@ def _read_channel(inventory_root, cha_element, _ns): | |||
unit=True) | |||
depth = _read_floattype(cha_element, _ns("depth"), Distance, | |||
unit=True) | |||
# Set depth to 0 if it is missing (#1816) | |||
if depth is None: | |||
depth = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same goes for longitude/latitude/elevation, they're all not mandatory in sc3ml according to the schema..
Also, please show a warning message, e.g.
if depth is None:
msg = "Channel is missing depth information, using '0.0'"
warnings.warn(msg)
depth = 0
obspy/io/seiscomp/sc3ml.py
Outdated
search = "responsePAZ[@publicID='" + response_id + "']" | ||
response_element = inventory_root.find(_ns(search)) | ||
if response_element is None: | ||
resp_type == 'ResponsePolynomial' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's also ResponseFIR
tags.. they should also be searched, no?
I'd also propose to sanitize the lookup a bit so that (potential/eventual/hypothetical) problems with one publicID being multiplied in the file are caught:
response_elements = []
for resp_type in ['responsePAZ', 'responsePolynomial', 'responseFIR']:
search = "{}[@publicID='{}']".format(resp_type, response_id)
response_elements += inventory_root.findall(_ns(search))
if len(response_elements) == 0:
msg = ("Could not find response tag with public ID "
"'{}'.".format(response_id))
raise ObsPyException(msg)
elif len(response_elements) > 1:
msg = ("Found multiple matching response tags with the same "
"public ID '{}'.".format(response_id))
raise ObsPyException(msg)
response_element = response_elements[0]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sensor response attribute can only be either responzePAZ or responsePolynomial. ResponseFIR does not need to be searched because it can only be defined in a digitalFilterChain
element inside a datalogger
element and by then you already know the type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK, thanks for looking into it.
@Jollyfant You are registered with |
You need to make sure that the sender address you're using is the one subscribed to the list.. |
I will fix and push this next week including the comments by @megies |
Hi guys, thanks so much for working on this. I just found this thread. @Jollyfant - I didn't get your private mail, but cheers for requesting the changes. |
Just a few flake8 fails that need to be fixed (lines too long) otherwise good to go, I think. |
These lines introduced a regression and would raise if the response was missing (i.e. channel level is requested) but has been fixed in the last commits.
I will do some more stability testing later and merge this by the end of this week. |
Thanks a bunch! Are you going to write the mailing list? |
Yeah but it's moderated so someone needs to approve it. |
Just did. Everything is moderated now as some spammers faked emails coming from us to post to the mailing list. A bit inconvenient that every message has to be approved but better than having the spam. I actually don't know how other mailing lists deal with this. |
That would be a good use case for email signing with PGP.. in principle.. ;-) |
Still needs tests.