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

mseed: Avoid invalid warning when guessing endianness of timestamps / UTCDateTime: proper exceptions on invalid 'julday' #1988

Merged
merged 7 commits into from May 9, 2018

Conversation

Projects
None yet
3 participants
@megies
Member

megies commented Nov 6, 2017

mseed: avoid showing invalid warnings when guessing endian during
parsing time stamps

so far, if during parsing a timestamp the endian is not known beforehand
and if the wrong endian is tried first, a warning message about
invalid fractional seconds gets shown which is a bogus warning

also, internally we simply rely on UTCDateTime.__init__() raising any
exception whatsoever for endian detection. this adds some sanity by
checking if the julday is in between 1-366 (which is the range accepted
by UTCDateTime.__init__)

utcdatetime: add proper sanity checks for UTCDateTime(..., julday=..)

right now passing invalid juldays can result in misleading error messages, because
julday is silently discarded when encountering out-of-bounds values that
internally in datetime.datetime.strptime(..., '%Y%j') raise "ValueError:
unconverted data remains:"

In [1]: UTCDateTime(year=2017, julday=367)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-0e1465633d82> in <module>()
----> 1 UTCDateTime(year=2017, julday=367)

/home/megies/git/obspy-master/obspy/core/utcdatetime.py in __init__(self, *args, **kwargs)
    357             kwargs['microsecond'] = int(round(_frac * 1e6))
    358             args = args[0:5]
--> 359         dt = datetime.datetime(*args, **kwargs)
    360         self._from_datetime(dt)
    361

TypeError: Required argument 'month' (pos 2) not found

This changes this to:

In [1]: UTCDateTime(year=2017, julday=367)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-1-0e1465633d82> in <module>()
----> 1 UTCDateTime(year=2017, julday=367)

/home/megies/git/obspy-master/obspy/core/utcdatetime.py in __init__(self, *args, **kwargs)
    342             if not (0 <= int(kwargs['julday']) <= 366):
    343                 msg = "'julday' out of bounds: {!s}".format(kwargs['julday'])
--> 344                 raise ValueError(msg)
    345             if 'year' in kwargs:
    346                 # year given as kwargs

ValueError: 'julday' out of bounds: 367

no time to add tests right now, sorry..

@megies megies added this to the 1.1.1 milestone Nov 6, 2017

@megies megies requested a review from krischer Nov 6, 2017

@megies megies changed the base branch from master to maintenance_1.1.x Nov 6, 2017

megies added a commit that referenced this pull request Dec 5, 2017

megies added a commit that referenced this pull request Dec 5, 2017

utcdatetime tests: change exception type
this used to raise TypeError but appropriate type clearly is ValueError
see #1988

@megies megies added the bug label Jan 8, 2018

@Jollyfant

This comment has been minimized.

Show comment
Hide comment
@Jollyfant

Jollyfant Apr 26, 2018

Contributor

Shouldn't the minimum be julday=1 instead of 0? Considering that the last index is 366.

Contributor

Jollyfant commented Apr 26, 2018

Shouldn't the minimum be julday=1 instead of 0? Considering that the last index is 366.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Apr 26, 2018

Member

Shouldn't the minimum be julday=1 instead of 0? Considering that the last index is 366.

You're right, we should be checking for 1 <= julday <= 366 👍

Member

megies commented Apr 26, 2018

Shouldn't the minimum be julday=1 instead of 0? Considering that the last index is 366.

You're right, we should be checking for 1 <= julday <= 366 👍

megies added a commit that referenced this pull request Apr 26, 2018

megies added a commit that referenced this pull request Apr 26, 2018

utcdatetime tests: change exception type
this used to raise TypeError but appropriate type clearly is ValueError
see #1988
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Apr 26, 2018

Member

Fixed that point and rebased.

Member

megies commented Apr 26, 2018

Fixed that point and rebased.

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer May 2, 2018

Member

needs to be rebased again

Member

krischer commented May 2, 2018

needs to be rebased again

megies added some commits Nov 6, 2017

mseed: avoid showing invalid warnings when guessing endian during
parsing time stamps

so far, if during parsing a timestamp the endian is not known beforehand
and if the wrong endian is tried first, a warning message about
invalid fractional seconds gets shown which is a bogus warning

also, internally we simply rely on UTCDateTime.__init__() raising any
exception whatsoever for endian detection. this adds some sanity by
checking if the julday is in between 0-366 (which is the range accepted
by UTCDateTime.__init__)
utcdatetime: add proper sanity checks for UTCDateTime(..., julday=..)
right now passing invalid juldays can result in misleading error messages, because
julday is silently discarded when encountering out-of-bounds values that
internally in datetime.datetime.strptime(..., '%Y%j') raise "ValueError:
unconverted data remains:"

In [1]: UTCDateTime(year=2017, julday=367)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-0e1465633d82> in <module>()
----> 1 UTCDateTime(year=2017, julday=367)

/home/megies/git/obspy-master/obspy/core/utcdatetime.py in __init__(self, *args, **kwargs)
    357             kwargs['microsecond'] = int(round(_frac * 1e6))
    358             args = args[0:5]
--> 359         dt = datetime.datetime(*args, **kwargs)
    360         self._from_datetime(dt)
    361

TypeError: Required argument 'month' (pos 2) not found

This changes this to:

In [1]: UTCDateTime(year=2017, julday=367)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-1-0e1465633d82> in <module>()
----> 1 UTCDateTime(year=2017, julday=367)

/home/megies/git/obspy-master/obspy/core/utcdatetime.py in __init__(self, *args, **kwargs)
    342             if not (0 <= int(kwargs['julday']) <= 366):
    343                 msg = "'julday' out of bounds: {!s}".format(kwargs['julday'])
--> 344                 raise ValueError(msg)
    345             if 'year' in kwargs:
    346                 # year given as kwargs

ValueError: 'julday' out of bounds: 367
utcdatetime tests: change exception type
this used to raise TypeError but appropriate type clearly is ValueError
see #1988
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 3, 2018

Member

Force pushed.. old hash was 746c8c8

Member

megies commented May 3, 2018

Force pushed.. old hash was 746c8c8

megies added some commits Apr 26, 2018

mseed time parsing: restore old behavior (to raise internal exception on
datetime.datetime.__init__ with invalid year when wrong byteorder is
used)
@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 4, 2018

Member

Good to merge I think @krischer

Member

megies commented May 4, 2018

Good to merge I think @krischer

@krischer

Looks mostly good. It would be nice to also add a test for the MiniSEED part - that module has become very complex over the years and I fear it will keep changing so a test would be appreciated.

Show outdated Hide outdated obspy/io/mseed/util.py
microsecond=msec % 1000000) + offset
except TypeError:
msg = 'Problem decoding time (wrong endian?)'
raise InternalMSEEDParseTimeError(msg)

This comment has been minimized.

@krischer

krischer May 4, 2018

Member

Maybe pass on the message of the exception which would ease debugging and improve understanding of whatever problem is encountered?

@krischer

krischer May 4, 2018

Member

Maybe pass on the message of the exception which would ease debugging and improve understanding of whatever problem is encountered?

This comment has been minimized.

@megies

megies May 7, 2018

Member

I don't think the message would be very helpful, at least not in the case that seems to be what is normally encountered (when trying the wrong endian):

$ python -c 'from obspy import UTCDateTime; UTCDateTime(year=444444, julday=10)'
Version 7.000kness 20eader in AFM: UnderlinePosition -133
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/megies/git/obspy/obspy/core/utcdatetime.py", line 377, in __init__
    dt = datetime.datetime(*args, **kwargs)
TypeError: Required argument 'month' (pos 2) not found

I'm not sure it would help to see that message to be honest.

@megies

megies May 7, 2018

Member

I don't think the message would be very helpful, at least not in the case that seems to be what is normally encountered (when trying the wrong endian):

$ python -c 'from obspy import UTCDateTime; UTCDateTime(year=444444, julday=10)'
Version 7.000kness 20eader in AFM: UnderlinePosition -133
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/megies/git/obspy/obspy/core/utcdatetime.py", line 377, in __init__
    dt = datetime.datetime(*args, **kwargs)
TypeError: Required argument 'month' (pos 2) not found

I'm not sure it would help to see that message to be honest.

This comment has been minimized.

@megies

megies May 7, 2018

Member

In any case this PR is not making things worse. Before it there was a bare try/except, now at least I'm only catching an exception that is explicitly raised inside the subroutine that parses the timestamp.

@megies

megies May 7, 2018

Member

In any case this PR is not making things worse. Before it there was a bare try/except, now at least I'm only catching an exception that is explicitly raised inside the subroutine that parses the timestamp.

endian = "<"
values = unpack(fmt(endian), data)
starttime = _parse_time(values)
else:
values = unpack(fmt(endian), data)
try:
starttime = _parse_time(values)
except Exception:
except InternalMSEEDParseTimeError:
msg = ("Invalid starttime found. The passed byte order is likely "
"wrong.")
raise ValueError(msg)

This comment has been minimized.

@krischer

krischer May 4, 2018

Member

again pass on exception message.

@krischer

krischer May 4, 2018

Member

again pass on exception message.

st2 = read(os.path.join(self.path,
'2011-09-06-1311-36S.A1032_001BH_Z.mseed'))
self.assertEqual(len(w), 1)
self.assertEqual(w[0].category, UserWarning)

This comment has been minimized.

@krischer

krischer May 4, 2018

Member

why is this no longer necessary?

@krischer

krischer May 4, 2018

Member

why is this no longer necessary?

This comment has been minimized.

@megies

megies May 7, 2018

Member

Reading this file triggers the invalid warning that I got rid of in mseed reading (i.e. potentially trialing the wrong endian in the first attempt).

The only thing that changes in here is that the warning-catching was removed, nothing else.

@megies

megies May 7, 2018

Member

Reading this file triggers the invalid warning that I got rid of in mseed reading (i.e. potentially trialing the wrong endian in the first attempt).

The only thing that changes in here is that the warning-catching was removed, nothing else.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 7, 2018

Member

also add a test for the MiniSEED part

What exactly do you want tested? Outwards facing, the only thing that changes in mseed is that the invalid warning went away, see the change in that seisan test. Sure, I could add a test that checks that no warnings a shown. But I don't see a lot of value in that.. it's kinda implicitly tested because if you run the tests you don't see a warning cluttering stdout.. ;-)

Other than that there's only changes in _parse_time() that could be tested, so you want a test for that internal routine (which didn't have tests yet and isn't even exposed on module level)?

Member

megies commented May 7, 2018

also add a test for the MiniSEED part

What exactly do you want tested? Outwards facing, the only thing that changes in mseed is that the invalid warning went away, see the change in that seisan test. Sure, I could add a test that checks that no warnings a shown. But I don't see a lot of value in that.. it's kinda implicitly tested because if you run the tests you don't see a warning cluttering stdout.. ;-)

Other than that there's only changes in _parse_time() that could be tested, so you want a test for that internal routine (which didn't have tests yet and isn't even exposed on module level)?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 9, 2018

Member

@krischer do you really want more changes in here or can i merge and do 1.1.1RC1?

Member

megies commented May 9, 2018

@krischer do you really want more changes in here or can i merge and do 1.1.1RC1?

@krischer

This comment has been minimized.

Show comment
Hide comment
@krischer

krischer May 9, 2018

Member

You are right - I misinterpreted what this PR is doing. Merging :-)

Member

krischer commented May 9, 2018

You are right - I misinterpreted what this PR is doing. Merging :-)

@krischer krischer merged commit 28d0df6 into maintenance_1.1.x May 9, 2018

4 of 7 checks passed

codecov/patch 87.5% of diff hit (target 90%)
Details
docker-deb-buildbot Deb packaging succeeded but tests failed
Details
docker-testbot Docker tests failed
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 88.08% (+1.11%) compared to 867c884
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@krischer krischer deleted the fix_mseed_avoid_invalid_warning branch May 9, 2018

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