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

Change time_error default for Catalog to NLLOC_OBS #2371 #2374

Conversation

Projects
None yet
3 participants
@LMurrayBergquist
Copy link
Contributor

commented Apr 4, 2019

This Pull Request changes the default time_error to 0.0 instead of -1 so that error will no longer be interpreted as 1 second.

It was initiated based on issue #2371 .

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 .

LMurrayBergquist added some commits Apr 4, 2019

Change in obspy/io/nlloc/core.py
Changes the default time_error to 0.0 instead of -1 so that error will no longer be interpreted as 1second.
Update CHANGELOG.txt
Added lines:
 "- obspy.io.mseed:
   * Add add ability to write int64 data to mseed if it can safely be downcast to int32 data, otherwise raises ObsPyMSEEDError. (see #2356)"
time_error = pick.time_errors.uncertainty or -1
if time_error == -1:
time_error = pick.time_errors.uncertainty or 0.0
if time_error == 0.0:

This comment has been minimized.

Copy link
@claudiodsf

claudiodsf Apr 5, 2019

Member

Is equality to floating 0.0 safe? Should it be 0 or is it the same?

This comment has been minimized.

Copy link
@claudiodsf

claudiodsf Apr 5, 2019

Member

Another possibility is:

time_error = pick.time_errors.uncertainty or None
if time_error is None:
  try:
    time_error = (pick.time_errors.upper_uncertainty +
                           pick.time_errors.lower_uncertainty) / 2.0
  except Exception:
    time_error = 0.0

This comment has been minimized.

Copy link
@megies

megies Apr 5, 2019

Member

Is equality to floating 0.0 safe? Should it be 0 or is it the same?

That's the same.. after all 0.0000000001 won't compare to integer 0, right? ;-)
Also I don't get the point, is there a floating point implementation that is not able to represent 0 exactly??

This comment has been minimized.

Copy link
@megies

megies Apr 5, 2019

Member

But yeah, without lookin at the code details your alternative might make sense (I don't know how the upp/lower is generally handled in there)

LMurrayBergquist added some commits Apr 6, 2019

Update obspy/io/nlloc/core.py
Changed so that if time_error is None and can't be found from upper/lower uncertainty it is set to 0.0 as an Exception as suggested by @claudiodsf
Change in obspy/io/nlloc/core.py
Changes the default time_error to 0.0 instead of -1 so that error will no longer be interpreted as 1second.
Update obspy/io/nlloc/core.py
Changed so that if time_error is None and can't be found from upper/lower uncertainty it is set to 0.0 as an Exception as suggested by @claudiodsf
@megies

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

@LMurrayBergquist please rebase on current master to get clean CI results. If you need help please meet up with @DominikStr.

LMurrayBergquist added some commits Apr 4, 2019

Change in obspy/io/nlloc/core.py
Changes the default time_error to 0.0 instead of -1 so that error will no longer be interpreted as 1second.
Update obspy/io/nlloc/core.py
Changed so that if time_error is None and can't be found from upper/lower uncertainty it is set to 0.0 as an Exception as suggested by @claudiodsf
@LMurrayBergquist

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Ok, I think I just rebased, let's see if that works now!

@claudiodsf

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Thanks for taking care of this. Good to go for me. 👍

@megies

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

Ok, I think I just rebased, let's see if that works now!

Nothing changed here, with rebasing, you have to kind of "overwrite" the history in here and git will only let you do this if you add -f or --force to your git push ....

LMurrayBergquist added some commits Apr 4, 2019

Change in obspy/io/nlloc/core.py
Changes the default time_error to 0.0 instead of -1 so that error will no longer be interpreted as 1second.
Update obspy/io/nlloc/core.py
Changed so that if time_error is None and can't be found from upper/lower uncertainty it is set to 0.0 as an Exception as suggested by @claudiodsf
@claudiodsf

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Hi, let me know if I can help rebasing and merging this PR.

@LMurrayBergquist

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@claudiodsf that could be helpful actually, I just tried again with "git rebase origin/master" which gives me a conflict in obspy/io/nlloc/core.py, which I resolve manually and write "git rebase --continue", but then it says that there is nothing to change and it won't continue...

@claudiodsf

This comment has been minimized.

Copy link
Member

commented May 10, 2019

@LMurrayBergquist can you give me write access to your local branch?

@LMurrayBergquist

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@LMurrayBergquist can you give me write access to your local branch?

Ok, I think I've added you so that you should have write access... I'm pretty new at this so maybe there's a better way to just give you write access to this branch, I'm not sure

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.