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 rotate to ZNE #1927

Merged
merged 16 commits into from Oct 14, 2017

Conversation

Projects
None yet
2 participants
@megies
Member

megies commented Oct 12, 2017

seems it did not work correctly for non-orthogonal input data

code change by @jwassermann

fixes #1913

fix rotate to ZNE
seems it did not work correctly for non-orthogonal input data

code change by @jwassermann

@megies megies added this to the 1.1.0 milestone Oct 12, 2017

@megies megies requested a review from krischer Oct 12, 2017

megies and others added some commits Oct 12, 2017

adjust one rotation test
testing for rtol 1e-10 is way enough, no need to test for strict equality
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 13, 2017

Member

Gonna add the test case highlighted in #1913 (comment) in a bit..

Member

megies commented Oct 13, 2017

Gonna add the test case highlighted in #1913 (comment) in a bit..

krischer added some commits Oct 13, 2017

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Oct 13, 2017

Member

So this is now one the best tested pieces of code in ObsPy given that it is only like 10 lines of logic... I cannot think of anything else to test and I would be pretty surprised if this is still wrong.

The good news is that it previously was also correct for orthogonal components which probably limits the damage this bug has done to some rare cases.

Thanks a lot to everyone who contributed!

Member

krischer commented Oct 13, 2017

So this is now one the best tested pieces of code in ObsPy given that it is only like 10 lines of logic... I cannot think of anything else to test and I would be pretty surprised if this is still wrong.

The good news is that it previously was also correct for orthogonal components which probably limits the damage this bug has done to some rare cases.

Thanks a lot to everyone who contributed!

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 14, 2017

Member

Yeehaw. Looks good to me. All tests (old and new) pass, so this should be good. Also @fbernauer stated that he checked the code change by @jwassermann locally and it works as expected.

I think this can be merged. Should we hit the big green button down there @krischer?

Member

megies commented Oct 14, 2017

Yeehaw. Looks good to me. All tests (old and new) pass, so this should be good. Also @fbernauer stated that he checked the code change by @jwassermann locally and it works as expected.

I think this can be merged. Should we hit the big green button down there @krischer?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Oct 14, 2017

Member

Merging this now. If there's really problems left (which I doubt) then we can handle it in a follow-up.

Member

megies commented Oct 14, 2017

Merging this now. If there's really problems left (which I doubt) then we can handle it in a follow-up.

@megies megies merged commit 4ff4650 into master Oct 14, 2017

5 of 6 checks passed

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

@megies megies deleted the fix_rotate_zne_non-ortho branch Oct 14, 2017

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