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 test_farfield_2xn_input #1642

Merged
merged 1 commit into from Feb 13, 2017

Conversation

Projects
None yet
4 participants
@seisman
Copy link
Contributor

commented Jan 14, 2017

This PR fixes two bugs in farfield unittest:

  1. The function farfield expects six component moment tensor in NED system, while GCMT gives moment tensor in RTP system. So a coordinate transformation from RTP to NED is needed.

  2. The 2xn vector [theta, phi] is in radian not in degree.

+TESTS: core.event.source

@krischer krischer requested review from ThomasLecocq and MMesch Jan 14, 2017

@megies

This comment has been minimized.

Copy link
Member

commented Jan 14, 2017

+TESTS: core.event.source

core tests are always executed, you only need this for "network" modules, that are not run by default, see https://github.com/obspy/obspy/blob/master/obspy/core/util/base.py#L45

@megies megies added this to the 1.0.3 milestone Jan 14, 2017

@krischer

This comment has been minimized.

Copy link
Member

commented Jan 30, 2017

@MMesch @ThomasLecocq Can one of you have a look and review this? Thanks! :-)

@krischer

This comment has been minimized.

Copy link
Member

commented Jan 30, 2017

Am I correct in assuming that the test just did not test something particularly useful and was documented wrongly but the actualy calculations were still right?

@megies

This comment has been minimized.

Copy link
Member

commented Feb 7, 2017

@MMesch @ThomasLecocq Can one of you have a look and review this? Thanks! :-)

ping?

@MMesch

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2017

Side question: should we (internally) prefer any particular convention for the moment tensor in obspy?

@megies

This comment has been minimized.

Copy link
Member

commented Feb 8, 2017

Side question: should we (internally) prefer any particular convention for the moment tensor in obspy?

Actually, Tensor and thus also MomentTensor in core.event.source are by definition of QuakeML Up-North-West? Generally, we could enhance these classes by making them coordinate system aware and capable of transforming themselves.. It would also be best to handle all moment tensors anywhere in our code base by such a proper object (as opposed to passing around numpy arrays)..

@MMesch

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2017

OK. I would propose to try using internally always the same convention, with one function that does the conversion at the beginning of a function call. Otherwise it will get confusing.

@MMesch

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2017

PS: if we use an object that problem would be fixed anyway

@megies

This comment has been minimized.

Copy link
Member

commented Feb 13, 2017

So, what about this one, @MMesch @ThomasLecocq? Calculations are correct looks like, just the test was wrong? Can we merge this to go forward with 1.0.3 or does this need more work?

@MMesch

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2017

I think it looks good! PS: didn't check the convention in detail.

@megies megies merged commit 68c0de8 into obspy:maintenance_1.0.x Feb 13, 2017

5 of 7 checks passed

docker-deb-buildbot Deb packaging succeeded but tests failed
Details
docker-testbot Docker tests failed
Details
codecov/changes Unable to determine changes, no report found at pull request base.
Details
codecov/patch 100% of diff hit (target 90.00%)
Details
codecov/project Absolute coverage decreased by -0.16% but relative coverage increased by +12.13% compared to d14c1f8
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@seisman seisman deleted the seisman:fix-farfield-doctest branch Feb 14, 2017

megies added a commit that referenced this pull request Feb 22, 2017

megies added a commit that referenced this pull request Feb 23, 2017

adjust tolerance for farfield test case (#1681)
* adjust tolerance for farfield test case

see #1642

see http://tests.obspy.org/70729/#1

* again try to fix farfield test case, see provious commit

* farfield test minor tolerance adjustments again
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.