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

Add SC3ML event read module #1848

Merged
merged 6 commits into from Aug 25, 2017

Conversation

Projects
None yet
4 participants
@Brtle
Contributor

Brtle commented Aug 8, 2017

I did the SC3ML event write module some time ago, here is the read module!

Once again, an XSLT is used to convert SC3ML file to a QuakeML file. This QuakeML file is then read with the QuakeML module. The original XSLT file was created by Stephan Herrnkind who kindly changed its license from GPL to LGPL to be compatible with the ObsPy license, thank you very much to him! I added some modifications which are indicated in the file itself.

I also bring some modifications to the write module. The version attribute have been added to the root node (optional but used in the _is_sc3ml method). The unit tests have been modified a bit to be more homogeneous with the tests of the read module. The _is_sc3ml method can now receive string parameter in addition to filename or file-like object (this test was failing randomly since the _is_sc3ml didn't accept string parameter). For that, I used the _xml_doc_from_anything method from the QuakeML module. Let me know if you prefere that I don't use this method since it's in another module.

Brtle added some commits Aug 3, 2017

@megies megies requested a review from Jollyfant Aug 8, 2017

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Aug 8, 2017

Member

Looks good from a first glance. @Jollyfant you're more into this topic I think, maybe you can give this a review when you have some free time?

One flake8 fail (see CircleCI log):

obspy/io/seiscomp/tests/test_event.py:134:9: N802 function name should be lowercase
Member

megies commented Aug 8, 2017

Looks good from a first glance. @Jollyfant you're more into this topic I think, maybe you can give this a review when you have some free time?

One flake8 fail (see CircleCI log):

obspy/io/seiscomp/tests/test_event.py:134:9: N802 function name should be lowercase
@Brtle

This comment has been minimized.

Show comment
Hide comment
@Brtle

Brtle Aug 8, 2017

Contributor

Trigger docs build +DOCS

Contributor

Brtle commented Aug 8, 2017

Trigger docs build +DOCS

@megies

This comment has been minimized.

Show comment
Hide comment
@megies
Member

megies commented Aug 8, 2017

@krischer

krischer approved these changes Aug 10, 2017 edited

Some very minor comments. I do like this approach of using a transform a lot! Thanks a lot for this!

Show outdated Hide outdated obspy/io/seiscomp/event.py
Show outdated Hide outdated obspy/io/seiscomp/sc3ml.py
Show outdated Hide outdated obspy/io/seiscomp/sc3ml.py
@Brtle

This comment has been minimized.

Show comment
Hide comment
@Brtle

Brtle Aug 11, 2017

Contributor

I just wanted to fix the regex and the assert and I finally refactored a little more…

I create a new file core.py where I put the common files to both event and inventory module. I fixed the _is_sc3ml method and I also factorize the validate method.

The _is_sc3ml function also no longer uses the optional version attribute but only the namespace.

Contributor

Brtle commented Aug 11, 2017

I just wanted to fix the regex and the assert and I finally refactored a little more…

I create a new file core.py where I put the common files to both event and inventory module. I fixed the _is_sc3ml method and I also factorize the validate method.

The _is_sc3ml function also no longer uses the optional version attribute but only the namespace.

@Jollyfant

This comment has been minimized.

Show comment
Hide comment
@Jollyfant

Jollyfant Aug 14, 2017

Contributor

That's a nice refactor, thanks. I'll try and review a bit more soon.

Contributor

Jollyfant commented Aug 14, 2017

That's a nice refactor, thanks. I'll try and review a bit more soon.

<EventParameters publicID="smi:www.iris.edu/ws/event/query">
<origin publicID="smi:www.iris.edu/ws/event/query?originId=7680412">
<time>
<value>2011-03-11T05:46:24.120000Z</value>
<value>2011-03-11T05:46:24.1200</value>

This comment has been minimized.

@Jollyfant

Jollyfant Aug 14, 2017

Contributor

Why is the time being changed?

@Jollyfant

Jollyfant Aug 14, 2017

Contributor

Why is the time being changed?

This comment has been minimized.

@Brtle

Brtle Aug 21, 2017

Contributor

The write module was tested by reading QuakeML files and writing SC3ML files using ObsPy read_events method. Now, the unit tests don't use the read_events method, the QuakeML file is directly converted in SC3ML with the XSLT file. I think it's more accurate since what we really want to test is the XSLT file. However, the result isn't exactly the same. The date format of the original file isn't modified when using the XSLT file.

By the way, sorry for the late answer!

@Brtle

Brtle Aug 21, 2017

Contributor

The write module was tested by reading QuakeML files and writing SC3ML files using ObsPy read_events method. Now, the unit tests don't use the read_events method, the QuakeML file is directly converted in SC3ML with the XSLT file. I think it's more accurate since what we really want to test is the XSLT file. However, the result isn't exactly the same. The date format of the original file isn't modified when using the XSLT file.

By the way, sorry for the late answer!

sc3ml_doc = _xml_doc_from_anything(filename)
quakeml_doc = transform(sc3ml_doc,
ID_PREFIX=etree.XSLT.strparam(id_prefix))
return Unpickler().load(io.BytesIO(quakeml_doc))
def _write_sc3ml(catalog, filename, validate=False, verbose=False,

This comment has been minimized.

@Jollyfant

Jollyfant Aug 14, 2017

Contributor

Instead of using open() it is usually better practise to use with open(). See at the bottom of this paragraph:

https://docs.python.org/2/tutorial/inputoutput.html#methods-of-file-objects

@Jollyfant

Jollyfant Aug 14, 2017

Contributor

Instead of using open() it is usually better practise to use with open(). See at the bottom of this paragraph:

https://docs.python.org/2/tutorial/inputoutput.html#methods-of-file-objects

This comment has been minimized.

@Brtle

Brtle Aug 21, 2017

Contributor

I don't see any open() in your quote. Moreover, the lxml FAQ states that:

if possible, prefer passing the file path directly into parse() instead of an opened Python file object.

Maybe you're speaking of this open(), which was copied from the QuakeML module. I can eventually use a with open() but I'll also modifie the QuakeML module then.

@Brtle

Brtle Aug 21, 2017

Contributor

I don't see any open() in your quote. Moreover, the lxml FAQ states that:

if possible, prefer passing the file path directly into parse() instead of an opened Python file object.

Maybe you're speaking of this open(), which was copied from the QuakeML module. I can eventually use a with open() but I'll also modifie the QuakeML module then.

This comment has been minimized.

@Jollyfant

Jollyfant Aug 21, 2017

Contributor

@Brtle yeah that's the one I was referring to. Sorry, I commented on the wrong function.

@Jollyfant

Jollyfant Aug 21, 2017

Contributor

@Brtle yeah that's the one I was referring to. Sorry, I commented on the wrong function.

This comment has been minimized.

@Brtle

Brtle Aug 21, 2017

Contributor

Done, I put a with open() in both SC3ML and QuakeML module.

@Brtle

Brtle Aug 21, 2017

Contributor

Done, I put a with open() in both SC3ML and QuakeML module.

@Jollyfant

This comment has been minimized.

Show comment
Hide comment
@Jollyfant

Jollyfant Aug 21, 2017

Contributor

Thanks! If CI pass it is good to merge IMHO. Can you merge it yourself @Brtle or no rights?

Contributor

Jollyfant commented Aug 21, 2017

Thanks! If CI pass it is good to merge IMHO. Can you merge it yourself @Brtle or no rights?

@Brtle

This comment has been minimized.

Show comment
Hide comment
@Brtle

Brtle Aug 21, 2017

Contributor

I don't have the right to merge it.

Contributor

Brtle commented Aug 21, 2017

I don't have the right to merge it.

@Jollyfant

This comment has been minimized.

Show comment
Hide comment
@Jollyfant

Jollyfant Aug 22, 2017

Contributor

Looks like Travis CI got stuck on the callback. But it looks like tests passed. @megies is it OK to ignore Travis and merge?

Contributor

Jollyfant commented Aug 22, 2017

Looks like Travis CI got stuck on the callback. But it looks like tests passed. @megies is it OK to ignore Travis and merge?

@Jollyfant Jollyfant merged commit 5838487 into obspy:master Aug 25, 2017

3 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
docs-buildbot Check out Pull Request docs build here:
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment