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 event support to SC3ML 0.10 schema #2024

Merged
merged 4 commits into from May 1, 2018

Conversation

Projects
None yet
3 participants
@Brtle
Contributor

Brtle commented Dec 8, 2017

Hey! I looked a bit to the SC3ML 0.10 event support (see #2004) and it should be pretty good. There are some changes on the Arrival/weight mapping and some fields have been added. Event types have also been added to better match QuakeML. The rest concerns the inventory part that I didn't touch.

I made some changes to the XSLT files provided by SeisComP3 and as they added them to their Github repository, I did a pull request. It is still open so changes can still be made to this PR.

Morevover, the SC3ML files are always written in SC3ML 0.10. I didn't let the possibility to write files in SC3ML 0.9. What do you think about that? Is it worth to keep the choice of the version at the cost of a slightly greater complexity? I do not know if many people want to write in an old SC3ML version.

@megies megies added this to the 1.2.0 milestone Dec 11, 2017

@megies megies added the .io label Dec 11, 2017

Show outdated Hide outdated obspy/io/seiscomp/core.py Outdated
@@ -116,7 +116,7 @@ def _write_sc3ml(catalog, filename, validate=False, verbose=False,
nsmap_ = getattr(catalog, "nsmap", {})
quakeml_doc = Pickler(nsmap=nsmap_).dumps(catalog)
xslt_filename = os.path.join(os.path.dirname(__file__), 'data',
'quakeml_1.2__sc3ml_0.9.xsl')
'quakeml_1.2__sc3ml_0.10.xsl')

This comment has been minimized.

@megies

megies Dec 11, 2017

Member

No idea.. do we want to be able to write different versions of SC3ML? Probably not and writing the most recent version is OK to keep maintenance down, I guess..

@megies

megies Dec 11, 2017

Member

No idea.. do we want to be able to write different versions of SC3ML? Probably not and writing the most recent version is OK to keep maintenance down, I guess..

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Dec 11, 2017

Member

Looks like you're changing all of the test files to SC3ML 0.10 in their header.. I think we might want to keep at least some of them at 0.9 so that reading that older version stays tested?

Member

megies commented Dec 11, 2017

Looks like you're changing all of the test files to SC3ML 0.10 in their header.. I think we might want to keep at least some of them at 0.9 so that reading that older version stays tested?

@Brtle

This comment has been minimized.

Show comment
Hide comment
@Brtle

Brtle Dec 13, 2017

Contributor

I think most of the unit tests should be done on the latest version. Note that all XSLT files are exactly the same, except the number version. However, it's true that there is a lack of tests for older versions. I added some unit tests to check the qml-example-1.2-RC3.sc3ml file with older versions. To avoid adding test files (which are already numerous) I modified the version dynamically for each test. Let me know what you think about it and if the old versions are enough tested.

Contributor

Brtle commented Dec 13, 2017

I think most of the unit tests should be done on the latest version. Note that all XSLT files are exactly the same, except the number version. However, it's true that there is a lack of tests for older versions. I added some unit tests to check the qml-example-1.2-RC3.sc3ml file with older versions. To avoid adding test files (which are already numerous) I modified the version dynamically for each test. Let me know what you think about it and if the old versions are enough tested.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Jan 24, 2018

Member

Sorry the slow response time - this looks pretty reasonable to me. Is this ready - then I'd give it a more detailed review.

Member

krischer commented Jan 24, 2018

Sorry the slow response time - this looks pretty reasonable to me. Is this ready - then I'd give it a more detailed review.

@Brtle

This comment has been minimized.

Show comment
Hide comment
@Brtle

Brtle Jan 24, 2018

Contributor

You can take a look! There are 2 things which can eventually be improved. The new SC3ML files are always written in SC3ML 0.10. Do you think we need to add the possibility to write 0.9 files? Moreover, I updated all the unit tests with the 0.10 schema. I added some unit tests to check old versions but it may not be enough. What do you think about it?

Contributor

Brtle commented Jan 24, 2018

You can take a look! There are 2 things which can eventually be improved. The new SC3ML files are always written in SC3ML 0.10. Do you think we need to add the possibility to write 0.9 files? Moreover, I updated all the unit tests with the 0.10 schema. I added some unit tests to check old versions but it may not be enough. What do you think about it?

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Jan 25, 2018

Member

You can take a look! There are 2 things which can eventually be improved. The new SC3ML files are always written in SC3ML 0.10. Do you think we need to add the possibility to write 0.9 files? Moreover, I updated all the unit tests with the 0.10 schema. I added some unit tests to check old versions but it may not be enough. What do you think about it?

In the tests for reading the old versions you basically already write old SC3ML files. How about exposing that functionality to the _write_sc3ml() function and adding a version argument. Then you can also use this for the same tests and already have some reasonable coverage. All xsd schemas for all SC3ML versions are also already present and if you also use these in the tests I would say we could have quite some confidence that SC3ML writing works correctly. All of this seems to pretty easy to do with the XSLT approach so I'd say go for it!

The new fields in SC3ML 0.10 (I guess its mainly the pdf fields) could be to custom tags in a separate namespace (see e.g. here for an example: http://docs.obspy.org/tutorial/code_snippets/quakeml_custom_tags.html). At that point ObsPy could directly read/write it. This would require some modification to the XSLT files and I don't have enough experience with these files to judge the effort.

Member

krischer commented Jan 25, 2018

You can take a look! There are 2 things which can eventually be improved. The new SC3ML files are always written in SC3ML 0.10. Do you think we need to add the possibility to write 0.9 files? Moreover, I updated all the unit tests with the 0.10 schema. I added some unit tests to check old versions but it may not be enough. What do you think about it?

In the tests for reading the old versions you basically already write old SC3ML files. How about exposing that functionality to the _write_sc3ml() function and adding a version argument. Then you can also use this for the same tests and already have some reasonable coverage. All xsd schemas for all SC3ML versions are also already present and if you also use these in the tests I would say we could have quite some confidence that SC3ML writing works correctly. All of this seems to pretty easy to do with the XSLT approach so I'd say go for it!

The new fields in SC3ML 0.10 (I guess its mainly the pdf fields) could be to custom tags in a separate namespace (see e.g. here for an example: http://docs.obspy.org/tutorial/code_snippets/quakeml_custom_tags.html). At that point ObsPy could directly read/write it. This would require some modification to the XSLT files and I don't have enough experience with these files to judge the effort.

@Brtle

This comment has been minimized.

Show comment
Hide comment
@Brtle

Brtle Jan 29, 2018

Contributor

In the tests for reading the old versions you basically already write old SC3ML files. How about exposing that functionality to the _write_sc3ml() function and adding a version argument. Then you can also use this for the same tests and already have some reasonable coverage. All xsd schemas for all SC3ML versions are also already present and if you also use these in the tests I would say we could have quite some confidence that SC3ML writing works correctly. All of this seems to pretty easy to do with the XSLT approach so I'd say go for it!

The point is that I juste change the namespace. The qml-example-1.2-RC3.sc3ml is actually the same for every SC3ML version, except the namespace part. I just wanted to test that old versions conversion didn't crash without testing every distinctive feature of each version. If we add the possibility to change the namespace in the _write_sc3ml function, this can generate unvalid SC3ML files. This can be done but a warning message should be displayed.

What we can do is maybe test all the versions that a unit test can run, for instance:

def test_read_xslt_origin(self, version=['0.9', '0.10']):
    …

def test_read_xslt_magnitude(self, version=['0.8', '0.9', '0.10']):
    …

So old versions should be relatively well tested without exploding the number of unit tests.

The new fields in SC3ML 0.10 (I guess its mainly the pdf fields) could be to custom tags in a separate namespace (see e.g. here for an example: http://docs.obspy.org/tutorial/code_snippets/quakeml_custom_tags.html). At that point ObsPy could directly read/write it. This would require some modification to the XSLT files and I don't have enough experience with these files to judge the effort.

The namespace is hardcoded in the XSLT files so we can't use the same XSLT file without templating the file itself. The easiest solution seems to keep one file per version. However, we will need to maintain several XSLT files when most people will likely use the latest version. It's easier to maintain in the read plugin since only the namespace change between all sc3ml_0.*__quakeml_1.2.xsl files.

Contributor

Brtle commented Jan 29, 2018

In the tests for reading the old versions you basically already write old SC3ML files. How about exposing that functionality to the _write_sc3ml() function and adding a version argument. Then you can also use this for the same tests and already have some reasonable coverage. All xsd schemas for all SC3ML versions are also already present and if you also use these in the tests I would say we could have quite some confidence that SC3ML writing works correctly. All of this seems to pretty easy to do with the XSLT approach so I'd say go for it!

The point is that I juste change the namespace. The qml-example-1.2-RC3.sc3ml is actually the same for every SC3ML version, except the namespace part. I just wanted to test that old versions conversion didn't crash without testing every distinctive feature of each version. If we add the possibility to change the namespace in the _write_sc3ml function, this can generate unvalid SC3ML files. This can be done but a warning message should be displayed.

What we can do is maybe test all the versions that a unit test can run, for instance:

def test_read_xslt_origin(self, version=['0.9', '0.10']):
    …

def test_read_xslt_magnitude(self, version=['0.8', '0.9', '0.10']):
    …

So old versions should be relatively well tested without exploding the number of unit tests.

The new fields in SC3ML 0.10 (I guess its mainly the pdf fields) could be to custom tags in a separate namespace (see e.g. here for an example: http://docs.obspy.org/tutorial/code_snippets/quakeml_custom_tags.html). At that point ObsPy could directly read/write it. This would require some modification to the XSLT files and I don't have enough experience with these files to judge the effort.

The namespace is hardcoded in the XSLT files so we can't use the same XSLT file without templating the file itself. The easiest solution seems to keep one file per version. However, we will need to maintain several XSLT files when most people will likely use the latest version. It's easier to maintain in the read plugin since only the namespace change between all sc3ml_0.*__quakeml_1.2.xsl files.

@Brtle

This comment has been minimized.

Show comment
Hide comment
@Brtle

Brtle Mar 5, 2018

Contributor

Sorry, it's been a little too long since I did not look into this PR!

I did some modifications to the unit tests. For each read conversion tests, every SC3ML versions for which the file is valid are tested. So old versions are not completly tested but I think this is the best compromise we can do.

I didn't change the _write_sc3ml method. I think it can be misleading if we add a parameter version which only change the namespace as I did in the unit tests. Invalid SC3ML files could be generated.

The new fields in SC3ML 0.10 (I guess its mainly the pdf fields) could be to custom tags in a separate namespace (see e.g. here for an example: http://docs.obspy.org/tutorial/code_snippets/quakeml_custom_tags.html). At that point ObsPy could directly read/write it. This would require some modification to the XSLT files and I don't have enough experience with these files to judge the effort.

I had answered a little quickly in my last reply and I did not quite understand it. But you're right, these custom tags should be used during the conversion! When I made the plugin the first time I was not aware of that. Actually there are other nodes for which it could be used. However, this require some upstream modifications to the XSLT files. Since this doesn't seem completely necessary and it does not only concern the 0.10 schema, it would be perhaps more judicious to make another PR?

Contributor

Brtle commented Mar 5, 2018

Sorry, it's been a little too long since I did not look into this PR!

I did some modifications to the unit tests. For each read conversion tests, every SC3ML versions for which the file is valid are tested. So old versions are not completly tested but I think this is the best compromise we can do.

I didn't change the _write_sc3ml method. I think it can be misleading if we add a parameter version which only change the namespace as I did in the unit tests. Invalid SC3ML files could be generated.

The new fields in SC3ML 0.10 (I guess its mainly the pdf fields) could be to custom tags in a separate namespace (see e.g. here for an example: http://docs.obspy.org/tutorial/code_snippets/quakeml_custom_tags.html). At that point ObsPy could directly read/write it. This would require some modification to the XSLT files and I don't have enough experience with these files to judge the effort.

I had answered a little quickly in my last reply and I did not quite understand it. But you're right, these custom tags should be used during the conversion! When I made the plugin the first time I was not aware of that. Actually there are other nodes for which it could be used. However, this require some upstream modifications to the XSLT files. Since this doesn't seem completely necessary and it does not only concern the 0.10 schema, it would be perhaps more judicious to make another PR?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 5, 2018

Member

and it does not only concern the 0.10 schema, it would be perhaps more judicious to make another PR?

yeah, sounds like it can be done in a separate PR if you prefer not to work on it right now

Member

megies commented Mar 5, 2018

and it does not only concern the 0.10 schema, it would be perhaps more judicious to make another PR?

yeah, sounds like it can be done in a separate PR if you prefer not to work on it right now

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Apr 27, 2018

Member

This looks ready @Brtle? Anything left to do in here?

Member

megies commented Apr 27, 2018

This looks ready @Brtle? Anything left to do in here?

@Brtle

This comment has been minimized.

Show comment
Hide comment
@Brtle

Brtle Apr 27, 2018

Contributor

Yes, it's ready for me! Do not hesitate if you have suggestions. I started looking for the custom tags but it will be in another PR.

Contributor

Brtle commented Apr 27, 2018

Yes, it's ready for me! Do not hesitate if you have suggestions. I started looking for the custom tags but it will be in another PR.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer May 1, 2018

Member

Thanks for this! I'll add a changelog entry directly to the master branch.

Member

krischer commented May 1, 2018

Thanks for this! I'll add a changelog entry directly to the master branch.

@krischer krischer merged commit aa0f955 into master May 1, 2018

5 of 6 checks passed

docker-testbot docker testbot results not available yet
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 95.34% of diff hit (target 90%)
Details
codecov/project 88.66% (+1.67%) compared to 1f76dff
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@krischer krischer deleted the sc3ml_0.10_event branch May 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment