Skip to content
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

Fix paz poles zeros reading #2260

Merged
merged 6 commits into from Dec 12, 2018

Conversation

Projects
None yet
4 participants
@thefroid
Copy link
Contributor

commented Nov 8, 2018

What does this PR do?

Fix a bug in the module read_inventory when a SC3ML file is given:
-Read the poles and zeros even if multiple whitespaces are present between poles and/or zeros
in the sc3ml file.

Why was it initiated? Any relevant Issues?

Unable to read the instrumental response of a raspberryshake3d (version5) with the module read_inventory.

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • This PR is not directly related to an existing issue (which has no PR yet).
  • If the PR is making changes to documentation, docs pages can be built automatically.
    Just remove the space in the following string after the + sign: "+ DOCS"
  • If any network modules should be tested for the PR, add them as a comma separated list
    (e.g. clients.fdsn,clients.arclink) after the colon in the following magic string: "+TESTS:"
    (you can also add "ALL" to just simply run all tests across all modules)
  • All tests still pass.
  • Any new features or fixed regressions are be covered via new tests.
  • Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • First time contributors have added your name to CONTRIBUTORS.txt .
@Jollyfant

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

Thanks! Did you edit one of the test files to include a poles & zeros field with multiple spaces? It's hard to see.

@thefroid

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

Hello,
Yes, in the file EB_response_sc3ml, i introduced two white-spaces between the zeros: (-5907,-3411) and (-5907, 3411) and between the poles: (-0.037, 0.037) and (-0.037,-0.037).

@Jollyfant

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

Let's wait for CI to work again and we can merge it @megies

@megies

megies approved these changes Nov 21, 2018

Copy link
Member

left a comment

Looks good to me, thx @thefroid. Anybody know what the xml standard has to say about this? Does it allow multiple spaces (I guess so..)? But I don't see how this can hurt in any case, so good to go as far as I'm concerned.

@megies megies added this to the 1.1.1 milestone Nov 21, 2018

@megies megies added the .io label Nov 21, 2018

@QuLogic

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

It is defined here. All whitespace locations are *, which means any number of them are allowed, including 0. I think this means that the code both before and after is still buggy.

There could be no spaces between each element (i.e., (1,2)(3,4)) which would never split either before or after this. There could also be spaces within the element (i.e., ( 1 , 2 )) which would be split too much both before and after.

So this change is good, but incomplete, though that's not the reporter's fault. The original code was not quite correct.

@megies

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

Nice catch there @QuLogic!

Maybe we need to use some impossible to read regex here? ;-)

In [26]: poles = "  (-0.037,0.037)  (-0.037,-0.037) (-6909,     9208)( -6909  ,-9208)  "

In [27]: pattern = r'[ ]*\([ ]*([\-0-9\.]+)[ ]*,[ ]*([\-0-9\.]+)[ ]*\)[ ]*'

In [28]: re.subn(pattern, r'\1 \2 ', poles)
Out[28]: ('-0.037 0.037 -0.037 -0.037 -6909 9208 -6909 -9208 ', 4)

..or maybe something less sophisticated but far more readable.. ;-)

In [33]: re.sub(r'[(),]', r' ', poles).split()
Out[33]: ['-0.037', '0.037', '-0.037', '-0.037', '-6909', '9208', '-6909', '-9208']

In [35]: np.array(re.sub(r'[(),]', r' ', poles).split(), dtype=np.float64)
Out[35]: 
array([ -3.70000000e-02,   3.70000000e-02,  -3.70000000e-02,
        -3.70000000e-02,  -6.90900000e+03,   9.20800000e+03,
        -6.90900000e+03,  -9.20800000e+03])
@megies

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

In any case, it might be good to do an assert afterwards that number of numbers matches opening brackets or something as well..

@QuLogic

This comment has been minimized.

Copy link
Member

commented Nov 28, 2018

If we aren't trying to verify and assume the thing is valid, then a much simpler regex would work: re.finditer(r'\(\s*([^,\s]+)\s*,\s*([^)\s]+)\s*\)', poles)

In [1]: poles = "  (-0.037,0.037)  (-0.037,-0.037) (-6909,     9208)( -6909  ,-9208)  "

In [2]: import re

In [3]: re.findall(r'\(\s*([^,\s]+)\s*,\s*([^)\s]+)\s*\)', poles)
Out[3]: 
[('-0.037', '0.037'),
 ('-0.037', '-0.037'),
 ('-6909', '9208'),
 ('-6909', '-9208')]

thefroid and others added some commits Nov 8, 2018

@megies megies force-pushed the thefroid:fix_PAZ_poles_zeros_reading branch from b11c6f7 to 9091f52 Dec 12, 2018

@megies

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

@thefroid I added some more commits to your PR, think this should be ready to merge then. I also needed to rebase and force-push to get rid of merge conflicts. Hope that's OK for you.

@megies megies merged commit 595d4fd into obspy:maintenance_1.1.x Dec 12, 2018

0 of 4 checks passed

ci/circleci Your tests failed on CircleCI
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
docker-testbot docker testbot results not available yet
@megies

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Fails are unrelated.

@megies

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

Thanks again, @thefroid!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.