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.strftime() raises ValueError with year <1900 #2167

Merged
merged 12 commits into from Feb 15, 2019

Conversation

Projects
4 participants
@megies
Copy link
Member

megies commented Jun 5, 2018

What does this PR do?

Trying to fix UTCDateTime.strftime() which raises ValueError in datetime.datetime.strftime() if year is <1900.

This currently fixes the problem for e.g. .strftime('%Y-%m-%d') by using simple string formatting instead of datetime.datetime.strftime().

Why was it initiated? Any relevant Issues?

Plotting catalogs that have events with origin time <1900 currently does not work.

PR Checklist

  • Correct base branch selected? master for new features, maintenance_... for bug fixes
  • All tests still pass.
  • Significant changes have been added to CHANGELOG.txt .

@megies megies added this to the 1.1.1 milestone Jun 5, 2018

@megies megies force-pushed the fix_catalog_plot_1900 branch from cf63fc9 to edb0535 Jun 5, 2018

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Jun 5, 2018

Just using str(dt) should fix this as implemented in #1325.

Also see #2013, #2015, #1318.

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Jun 5, 2018

We cannot really fix .strftime() on Python 2 aside from reimplementing it from scratch which we really don't want I think. How about just raising a nice error on Python 2 when calling UTCDateTime.strftime() with a year < 1900 and make the user's aware of the issue?

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Jun 5, 2018

I started fixing that inside the catalog plot routine by using string formatting, but then I thought it's a low hanging fruit at least for some cases and it might be useful for users as well to implement some easy workarounds inside UTCDateTime.strftime.. dunno

But yeah, if you think it's better to keep this out, fine with me as well

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Jun 7, 2018

Just using str(dt) should fix this as implemented in #1325.

I just had a quick look at this, and actually I think the fix proposed here is the way to go.

  • it's really only superseding datetime.datetime.strftime when that particular exception is hit
  • replacing the strftime commands in all relevant places (mostly where origin times are converted to strings) would introduce roughly 10 changes scattered across the code base, which will be obsolete after dropping python 2
  • this change can easily be reverted if necesssary when dropping Python 2
  • this change also enables users to use t.strftime() with times before 1900AD in user land codes on Python 2
t = UTCDateTime(998, 11, 9, 1, 39, 37)
self.assertEqual(t.strftime('%Y-%m-%d'), '0998-11-09')
# some things we can't easily fix by string formatting alone..
with self.assertRaises(ValueError) as context:

This comment has been minimized.

@calum-chamberlain

calum-chamberlain Jun 29, 2018

Contributor

It looks like, as written, this test only applies to Python < 3.2, and the error isn't raised on the appveyor Python 3.6 resulting in a failed test.

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Jul 2, 2018

Aside from the CI failures this seems to be a in a pretty good state. Can you maybe add a few more tests for the _strftime_replacement() method? This is easy enough to do just to make sure it works as intended even though is rarely actually called.

@megies megies referenced this pull request Oct 26, 2018

Open

Dropping Python 2 #2228

0 of 16 tasks complete

@krischer krischer force-pushed the fix_catalog_plot_1900 branch from 5044806 to ab946a4 Feb 14, 2019

@krischer krischer added this to Waiting for CI in Release 1.1.1 Feb 14, 2019

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Feb 14, 2019

Traceback (most recent call last):
File "/home/travis/miniconda/envs/test-environment/lib/python3.7/site-packages/obspy/core/tests/test_utcdatetime.py", line 1136, in test_strftime_with_years_less_than_1900
self.assertEqual(t.strftime('%Y-%m-%d'), '0998-11-09')
AssertionError: '998-11-09' != '0998-11-09'
- 998-11-09
+ 0998-11-09
? +

only happens on Linux...

the MacOS eerrors with Taup are unrelated.

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Feb 14, 2019

@krischer

This comment has been minimized.

Copy link
Member

krischer commented Feb 14, 2019

Not a bug according to the Python devs: https://bugs.python.org/issue32195

So this works on linux and python 3:

datetime.datetime(1, 1, 1).strftime("%04Y")

So I guess what we have to do it test for linux and Python >= 3.5 and do:

format = format.replace("%Y", "%04Y")

@ThomasLecocq ThomasLecocq moved this from Waiting for CI to In progress in Release 1.1.1 Feb 14, 2019

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Feb 14, 2019

side note, this failed

In [2]: UTCDateTime("998-01-01")
Out[2]: 9980-10-10T00:00:00.000000Z

In [3]: UTCDateTime("0998-01-01")
Out[3]: 0998-01-01T00:00:00.000000Z

and is corrected by a regex check, considering that the first part of a -, / or comma separated string is the year.

@ThomasLecocq

This comment has been minimized.

Copy link
Contributor

ThomasLecocq commented Feb 14, 2019

we regex check and reject the string years not being 4 digits

@ThomasLecocq ThomasLecocq moved this from In progress to Waiting for CI in Release 1.1.1 Feb 14, 2019

megies and others added some commits Jun 5, 2018

UTCDateTime.strftime: work around some problems when year is <1900
datetime.datetime.strftime() refuses to work with years <1900, so one
obvious simple workaround is to mimic at least those strftime commands
that are easily replaced by simple string formatting
utcdatetime: fix test, strftime with year < 1900 only fails on Python
2.7 for us, it was fixed in Python 3.2 and upwards

krischer added some commits Feb 14, 2019

@megies megies force-pushed the fix_catalog_plot_1900 branch from 5802db3 to 4f40051 Feb 14, 2019

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Feb 14, 2019

Rebased on current master and force-pushed so that we have fresh CI results tomorrow.

@megies

This comment has been minimized.

Copy link
Member Author

megies commented Feb 15, 2019

CI is green, only a minor flake8 thing and changelog missing which I'll add and then merge.

@megies megies merged commit b631158 into maintenance_1.1.x Feb 15, 2019

0 of 2 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@megies megies moved this from Waiting for CI to Done in Release 1.1.1 Feb 15, 2019

@QuLogic QuLogic deleted the fix_catalog_plot_1900 branch Feb 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.