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

UTCDateTime(year=..., julday=366) givin wrong output on non-leap years #2369

Open
wants to merge 6 commits into
base: master
from

Conversation

3 participants
@megies
Copy link
Member

commented Apr 4, 2019

Initializing UTCDateTime with UTCDateTime(year=..., julday=366) gives wrong result for non-leap years. It should instead raise the same exception as using UTCDateTime(year=..., julday=367) for leap years..

In [6]: UTCDateTime(year=2018, julday=366)
Out[6]: 2018-01-01T00:00:00.000000Z

In [7]: UTCDateTime(year=2017, julday=366)
Out[7]: 2017-01-01T00:00:00.000000Z

In [8]: UTCDateTime(year=2017, julday=365)
Out[8]: 2017-12-31T00:00:00.000000Z

In [9]: UTCDateTime(year=2018, julday=365)
Out[9]: 2018-12-31T00:00:00.000000Z

In [10]: UTCDateTime(year=2016, julday=365)
Out[10]: 2016-12-30T00:00:00.000000Z

In [11]: UTCDateTime(year=2016, julday=366)
Out[11]: 2016-12-31T00:00:00.000000Z

In [12]: UTCDateTime(year=2016, julday=367)
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-12-8f83b0252770> in <module>()
----> 1 UTCDateTime(year=2016, julday=367)

/home/megies/git/obspy/obspy/core/utcdatetime.pyc in __init__(self, *args, **kwargs)
    398             if not (1 <= int(kwargs['julday']) <= 366):
    399                 msg = "'julday' out of bounds: {!s}".format(kwargs['julday'])
--> 400                 raise ValueError(msg)
    401             if 'year' in kwargs:
    402                 # year given as kwargs

ValueError: 'julday' out of bounds: 367

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes (no more 1.1.x releases.)
  • This PR is not directly related to an existing issue (which has no PR yet).
  • [N/A] 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"
  • [N/A] 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.
  • [N/A] Any new or changed features have are fully documented.
  • Significant changes have been added to CHANGELOG.txt .
  • [N/A] First time contributors have added your name to CONTRIBUTORS.txt .

@megies megies added this to the 1.2.0 milestone Mar 29, 2019

@megies

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2019

@DominikStr you could work on this.

This has to changed to take into account leap years:

if not (1 <= int(kwargs['julday']) <= 366):
msg = "'julday' out of bounds: {!s}".format(kwargs['julday'])
raise ValueError(msg)

I think simplest would be to query calendar, I hope thats not problematic for execution speed, but since we use datetime extensively in there it shouldn't make a difference I think.

In [1]: import calendar

In [2]: calendar.isleap(2016)
Out[2]: True

In [3]: calendar.isleap(2017)
Out[3]: False

In [4]: calendar.isleap(2018)
Out[4]: False

In [5]: calendar.isleap(2020)
Out[5]: True
@DominikStr

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2019

(year % 4 == 0 and (year % 100 != 0 or year % 400 == 0) is exactly what calendar.isleap() is doing, so there is no need to import it.

There is probably no need for a test case?

DominikStr added a commit to DominikStr/obspy that referenced this pull request Apr 3, 2019

@megies

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

(year % 4 == 0 and (year % 100 != 0 or year % 400 == 0) is exactly what calendar.isleap() is doing, so there is no need to import it.

I'd still say using a function provided by the standard library is the preferred way to go. Custom code we have to maintain ourselves (even if this might never change unless the sun falls from the sky) but if we use standard library high level routines we don't have to care about maintenance ;-)

There is probably no need for a test case?

This is a dangerous thought.. the more often it comes up the sloppier everybody gets with test cases. Please add a simple regression test. Shouldn't take too long and everybody's happy. :-)

@megies

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

Converted into a PR..

@megies megies added this to In Progress in Release 1.2.0 Apr 4, 2019

DominikStr added a commit to DominikStr/obspy that referenced this pull request Apr 4, 2019

@DominikStr

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

Added a test case and use of the standard library. I think this should be finished now.

P.S.: I Have no idea why my commits for inventory.sort are appearing here.

@@ -402,11 +403,11 @@ def __init__(self, *args, **kwargs):
# year is first (and only) argument
year = args[0]
if not (1 <= int(kwargs['julday']) <= 366) and \
(year % 4 == 0 and (year % 100 != 0 or year % 400 == 0)):
calendar.isleap(year):

This comment has been minimized.

Copy link
@megies

megies Apr 5, 2019

Author Member

does this fit on the above line now?

Maybe it's a bit shorter and more readable sth like this?

days_in_year = calendar.isleap(year) and 365 or 366
if not (.... <= days_in_year):
    msg = ...
    raise ...
@krischer

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Something went wrong here - there are commits from two independent issues in this PR.

@DominikStr DominikStr force-pushed the DominikStr:fix_2369_UTC_Datetime_non_leap_years branch from c134704 to 7d681e1 Apr 27, 2019

DominikStr added a commit to DominikStr/obspy that referenced this pull request Apr 27, 2019

DominikStr added a commit to DominikStr/obspy that referenced this pull request Apr 27, 2019

@DominikStr DominikStr force-pushed the DominikStr:fix_2369_UTC_Datetime_non_leap_years branch from 7d681e1 to 5ac4eea Apr 27, 2019

DominikStr added a commit to DominikStr/obspy that referenced this pull request Apr 27, 2019

DominikStr added a commit to DominikStr/obspy that referenced this pull request Apr 27, 2019

@DominikStr DominikStr force-pushed the DominikStr:fix_2369_UTC_Datetime_non_leap_years branch from 5ac4eea to d9af287 Apr 27, 2019

@DominikStr

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

I think I based my branch on the sort branch. Now I cleaned everything up and the current branch should contain only commits relevant for this request.

@krischer

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Yes the commits look good now. Thanks for this. Let us know once this is ready for review!

@DominikStr

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

I would say this is ready. The bug is fixed and I think the test is sufficient.

megies added some commits May 15, 2019

final touches to UTCDateTime.__init__(..., julday=X) bug fixes
try/except/self.fail in tests isn't necessary. shorten the code a bit
and avoid duplication
@megies

This comment has been minimized.

Copy link
Member Author

commented May 15, 2019

I added some final touches, good to go if tests pass.

@megies megies moved this from In Progress to Waiting on CI in Release 1.2.0 May 15, 2019

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.