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

Taup geo bug #1278

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@MMesch
Contributor

MMesch commented Feb 20, 2016

this fixes a little bug in taup_geo. If the distance is larger than 180 degrees, the azimuth points in the wrong direction. Not sure if the fix works for distances > 360 degrees though. It is just a proposition without extensive testing.

MMesch added some commits Feb 20, 2016

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Feb 21, 2016

Member

Is this a bug fix that should go in 1.0.x?

Member

QuLogic commented Feb 21, 2016

Is this a bug fix that should go in 1.0.x?

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 21, 2016

Contributor

well right now the lon/lat paths are wrong for distances > 180 degrees. So I think as soon as possible but I don't know how you handle such things in obspy...

ps: I just thought that the great circle plotting routine might take a while to finish. That's why I put the bug fix here independently

Contributor

MMesch commented Feb 21, 2016

well right now the lon/lat paths are wrong for distances > 180 degrees. So I think as soon as possible but I don't know how you handle such things in obspy...

ps: I just thought that the great circle plotting routine might take a while to finish. That's why I put the bug fix here independently

@QuLogic QuLogic added this to the 1.0.x milestone Feb 21, 2016

@QuLogic

This comment has been minimized.

Show comment
Hide comment
@QuLogic

QuLogic Feb 21, 2016

Member

Can you add a test as well?

Member

QuLogic commented Feb 21, 2016

Can you add a test as well?

@QuLogic QuLogic added bug .taup labels Feb 21, 2016

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 22, 2016

Contributor

ok here is a test.

It does not test:

  • pierce points
  • distances > 360 degrees (what phase would we use for that?)
Contributor

MMesch commented Feb 22, 2016

ok here is a test.

It does not test:

  • pierce points
  • distances > 360 degrees (what phase would we use for that?)
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Feb 24, 2016

Member

@MMesch for future PRs: If it's a bug, it should go into maintenance_0.10.x (and branched off of it too, of course), see https://github.com/obspy/obspy/wiki/ObsPy-Git-Branching-Model.

But it's not a big deal here because master and maintenance_0.1.x haven't diverged significantly.. yet.

Since the base branch of a PR can not be changed after opening it, I would recommend the following (in order to not have another duplicate issue, with discussion split up among different tickets..):

  • continue fixing this issue here on your branch based on master
  • when ready to merge:
    • don't merge this into master
    • checkout another branch based on this branch (e.g. taup_bug2)
    • rebase that branch on maintenance_1.0.x
    • merge into maintenance_1.0.x
    • merge maintenance_1.0.x into master
    • close this PR

That way also the diff view in this PR is not lost.

Member

megies commented Feb 24, 2016

@MMesch for future PRs: If it's a bug, it should go into maintenance_0.10.x (and branched off of it too, of course), see https://github.com/obspy/obspy/wiki/ObsPy-Git-Branching-Model.

But it's not a big deal here because master and maintenance_0.1.x haven't diverged significantly.. yet.

Since the base branch of a PR can not be changed after opening it, I would recommend the following (in order to not have another duplicate issue, with discussion split up among different tickets..):

  • continue fixing this issue here on your branch based on master
  • when ready to merge:
    • don't merge this into master
    • checkout another branch based on this branch (e.g. taup_bug2)
    • rebase that branch on maintenance_1.0.x
    • merge into maintenance_1.0.x
    • merge maintenance_1.0.x into master
    • close this PR

That way also the diff view in this PR is not lost.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Feb 24, 2016

Member

Also, if this is something that might puzzle people who run into it a changelog entry might be good. If it's something that fails with a very clear information or Exception a changelog entry might not be necessary..

Member

megies commented Feb 24, 2016

Also, if this is something that might puzzle people who run into it a changelog entry might be good. If it's something that fails with a very clear information or Exception a changelog entry might not be necessary..

@MMesch

This comment has been minimized.

Show comment
Hide comment
@MMesch

MMesch Feb 24, 2016

Contributor

ok. I rebased it on maintenance. Should I open a new PR then?

Contributor

MMesch commented Feb 24, 2016

ok. I rebased it on maintenance. Should I open a new PR then?

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer Feb 24, 2016

Member

Yes. Github unfortunately does not allow to change the branch one wants to merge into :-(

Member

krischer commented Feb 24, 2016

Yes. Github unfortunately does not allow to change the branch one wants to merge into :-(

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Feb 24, 2016

Member

Ok, so closing as a duplicate of #1289.

Member

megies commented Feb 24, 2016

Ok, so closing as a duplicate of #1289.

@megies megies closed this Feb 24, 2016

@megies megies added the duplicate label Feb 24, 2016

@QuLogic QuLogic modified the milestones: 1.0.x, 1.0.1 Mar 23, 2016

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