SC3ML event read support for all schema versions #1873
Conversation
|
I'll take a look at it next week. However, I did some modifications to the original stylesheet when I did this module. It would be nice if these corrections were applied for older versions as well. I'll have to check that. |
Ah, I didn't know. Looks like most of them probably should be reported for inclusion upstream? |
Support versions from 0.5 to 0.9 for event and inventory read module. See #1873. Also fix the `_xml_doc_from_anything` to always return an lxml.etree.Element object.
|
I added the event read support for versions from 0.5 to 0.9. There was only 2 optional fields added ( The SC3ML 0.3 can't be converted with the current XSLT. There is some modifications to do but I'm not sure it's worth it. The 0.3 version seems quite old and a bit experimental since there is no 0.4 version. If someone ask, we can always implement it. Finally, I also modify the minimum usable version of the inventory read module. There is absolutely no change between version 0.5 and 0.7 so it's straightforward.
I send an email to the author when I did the event read module but I didn't get a reply. I will contact him again. |
Sounds OK for me. Whatever is simplest to achieve and maintain.
I agree, most likely not worth any additional efforts. That's really old and not widely encountered, I'd guess.
OK, good. I'll make this issue into a PR from your branch. |
As this now changes the author and date, I don't think this is a good idea any more. You can just put "Fixes #1873" in the description and have it close the issue automatically. |
I agree that this new behavior is now confusing and strange. I still don't like the idea of having twice as many tickets for the same amount of topics, though. In any case, this issue's OP was by me anyway, so no glitch was introduced at least in this issue by converting to a PR. |
That's interesting. Thanks for reviewing and editing the inventory too. |
|
@megies is there a way to reset Travis CI? |
obspy/io/seiscomp/event.py
Outdated
| warnings.warn('The file does not appear to be a SC3ML file. Proceed ' | ||
| 'with caution.') | ||
| version = SCHEMA_VERSION[-1] | ||
| else: |
There was a problem hiding this comment.
Should we raise a warning in this branch? e.g. Schema version %s is not supported, using the most recent version.
There was a problem hiding this comment.
A warning is already raised in the _is_sc3ml method. If we also put a warning here, there will be 2 warning messages.
There was a problem hiding this comment.
Maybe the warning should be moved from the _is method to the _read method? So that people that explicitly specify the file type during reading also see it?
There was a problem hiding this comment.
Done. I have also changed the warning to an exception.
Yep. But it's not necessary, the single fail is due to #1879 and it won't pass if I restart the build. Just curious, if you login to travis with github, don't you get the "restart build" buttons? If not, then being part of the developers team isn't enough for it, I guess.. :-/
|
|
Would be nice if you added a test that checks the warning message. Then you would also get green light from codecov which is complaining about to high percentage of uncovered lines in the diff, see https://codecov.io/gh/obspy/obspy/compare/f9a8a3fcdaca4c5a50e23155c001db76f2e61867...ee71bd14278d4153befb2aac044d89edc7242d88/diff#D14-89. Here's an example for a test that checks warning message: obspy/obspy/core/tests/test_trace.py Line 1338 in 91a3f29 |
Sorry for offtopic. But actually there is, I just haven't ever noticed it before. Ty! |
The XSLT conversion with a wrong namespace returns an empty SC3ML. Since there isn't an EventParameters node, the QuakeML module raise an exception. So The warnings have also been replaced by exceptions. See #1873.
|
I changed warnings to exceptions when the namespace was wrong since it always generates an empty file: <?xml version="1.0" encoding="UTF-8"?>
<q:quakeml xmlns:q="http://quakeml.org/xmlns/quakeml/1.2" xmlns="http://quakeml.org/xmlns/bed/1.2"/>There isn't an |
The XSLT conversion with a wrong namespace returns an empty SC3ML. Since there isn't an EventParameters node, the QuakeML module raise an exception. So The warnings have also been replaced by exceptions. See #1873.
Brtle
force-pushed
the
sc3ml_version_support
branch
from
0889d21
to
db86120
Compare
Brtle
force-pushed
the
sc3ml_version_support
branch
from
58ecc84
to
97819e6
Compare
There was a problem hiding this comment.
PR looks good to me. 100% of diff covered as well. If nobody else comes up with any more change requests, I'd propose to merge this soonish.
|
I just realized this does not need a new changelog entry as SC3ML support was not in 1.0.3 and the new support is already in the changelog. Thus I hit the big merge button. Thanks a bunch for this! |
|
Thanks a lot, @Brtle! |
Support versions from 0.5 to 0.9 for event and inventory read module. See obspy#1873. Also fix the `_xml_doc_from_anything` to always return an lxml.etree.Element object.
The XSLT conversion with a wrong namespace returns an empty SC3ML. Since there isn't an EventParameters node, the QuakeML module raise an exception. So The warnings have also been replaced by exceptions. See obspy#1873.
Read support for SC3ML event data was added in #1848 by @Brtle. However it restricts read support to schema version 0.9 currently. Since stylesheets provided by upstream maintainers (http://geofon.gfz-potsdam.de/schema/) are used for the conversion to QuakeML, it is trivial to support all schema versions, like also done in the SC3ML inventory read routine. I suppose there's a wide variety of SeisComp installations out there and thus also SC3ML files at different schema versions.
Since you did #1848 already, can I task you with this trivial extension, @Brtle?
I'd like to have this in 1.1.0 since it might be useful for quite a few people. Do you have some time to work on this in the next weeks? (I've added you to developers, so ideally work in a branch in the main repo for easier collaboration)