Skip to content

BUG: handle outofbounds datetimes in DatetimeConverter #13801

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

Conversation

jorisvandenbossche
Copy link
Member

xref #2579

This at least solves the direct negative consequence (erroring code by importing pandas) of registering our converters by default.

@jorisvandenbossche
Copy link
Member Author

@codecov-io
Copy link

codecov-io commented Jul 26, 2016

Current coverage is 85.23% (diff: 100%)

Merging #13801 into master will increase coverage by <.01%

@@             master     #13801   diff @@
==========================================
  Files           140        140          
  Lines         50415      50415          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          42970      42972     +2   
+ Misses         7445       7443     -2   
  Partials          0          0          

Powered by Codecov. Last update 98c5b88...6b6b08e

@@ -1222,6 +1222,14 @@ def test_secondary_y_irregular_ts_xlim(self):
self.assertEqual(left, ts_irregular.index.min().toordinal())
self.assertEqual(right, ts_irregular.index.max().toordinal())

def test_plot_outofbounds_datetime(self):
Copy link
Contributor

@tacaswell tacaswell Jul 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does pandas have the equivalent of @cleanup from the mpl tests? This is touching global state and can leak to other tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In TestPlotBase (from which this testcase inherits), all settings are set to the defaul in the setup, and the figures closed in the teardown.
Or are there still other things that may leak?

@ocehugo
Copy link

ocehugo commented Aug 10, 2016

working fine ;). No conflict with matplotlib so far in my usage. thanks for that!

@jorisvandenbossche
Copy link
Member Author

@sinhrks @TomAugspurger OK to merge?

@@ -216,7 +216,7 @@ def try_parse(values):
else:
values = [_dt_to_float_ordinal(x) for x in values]
except Exception:
pass
values = _dt_to_float_ordinal(values)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow me I do not understand converter dispatch logic.

Might be unrelated to the PR, but is it possible that input can have a type not registered for DatetimeConverter like below condition? If so, it may better to box the above with try-except to return input as it is.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not completely sure, but by letting the DatetimeConverter accept for example strings (although string type is not registered), this makes it possible to do ax.set_xlim('2012-01-01', '2012-01-10')

@jorisvandenbossche
Copy link
Member Author

@sinhrks further comments? If not, be my guest to merge :-)

@sinhrks sinhrks closed this in 5d791cc Aug 16, 2016
@sinhrks
Copy link
Member

sinhrks commented Aug 16, 2016

thx!

@jorisvandenbossche jorisvandenbossche deleted the plot-datetime-converter branch January 6, 2017 12:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants