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

is_holiday function doesn't return correct result when pandas Timestamp is passed. #294

Closed
yassineAlouini opened this issue Nov 23, 2018 · 7 comments
Projects

Comments

@yassineAlouini
Copy link

The is_holiday function expects a datetime.datetime format. However, it doesn't raise any error when a pandas Timestamp is passed (https://pandas.pydata.org/pandas-docs/version/0.23.4/generated/pandas.Timestamp.html).

Here is a code example (workalendar version '3.1.1'):

from workalendar import europe
import pandas as pd
import datetime


fr = europe.France()
# Should be True and returns True
print(fr.is_holiday(datetime.datetime(2017, 1, 1)))
# Should be True but returns False
print(fr.is_holiday(pd.to_datetime("2017-1-1")))
# To fix this, use the `to_pydatetime` method
print(fr.is_holiday(pd.to_datetime("2017-1-1").to_pydatetime()))

I expect the library to raise an error when the passed input isn't of type datetime.datetime.
What are your thoughts?
Thanks in advance. :)

@brunobord
Copy link
Member

mmmm.... this is sad that padas Timestamp doesn't inherit from the stdlib types...
If there's a way to convert from pandas classes to standard types, that could be doable. Although I wouldn't like to add a dependency to pandas in the project (only in tests it would be fine).

@yassineAlouini
Copy link
Author

yassineAlouini commented Nov 23, 2018

@brunobord Yes, it is possible using the .to_pydatetime() method:

import pandas as pd
tms = pd.to_datetime('2017-1-1')
print(type(tms))
# pandas._libs.tslibs.timestamps.Timestamp
print(type(tms.to_pydatetime()))
# datetime.datetime

@brunobord
Copy link
Member

ok, looks like it's doable.

Then my question would be: why should workalendar accept pandas Timestamps? I've had an afterthought and I'm not sure workalendar would support exotic date/datetime input types.
If we accept pandas timestamps, why not accepting strings like YYYY-MM-DD, and then integers (as a unix timestamp), and then a picture of a calendar as an input? (yes, I'm exaggerating, for the sake of the argument).
I'm not saying that I want to reject this possibility, I'm saying that I think I'd need a serious argument to accept non-standard types in our API.

@yassineAlouini
Copy link
Author

I see your point.

I am not advising to accept additional types. Maybe a check of type + raising an exception would be enough to avoid dealing with other formats than datetime.datetime?

What do you think?

@johnberroa
Copy link

I had the same problem. Throwing an error would be nice because if I didn't double check I would have never known.

@brunobord brunobord added this to Working in Workalendar Nov 29, 2018
@brunobord
Copy link
Member

turns out pandas' Timeout is a datetime subclass. Interesting, we may be able to support more types, as long as they are something like "duck dates" (that quack like a date)

brunobord added a commit that referenced this issue Nov 29, 2018
* only standard library `date` and `datetime` types (or subtypes) are supported,
* added a new dedicated exception,
* added a paragraph in the documentation about this type support.

refs #294
brunobord added a commit that referenced this issue Nov 29, 2018
* only standard library `date` and `datetime` types (or subtypes) are supported,
* added a new dedicated exception,
* added a paragraph in the documentation about this type support.

refs #294
@brunobord
Copy link
Member

closed by #298 + released with the version 3.2.0

@brunobord brunobord moved this from Working to Done in Workalendar Nov 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Workalendar
  
Done/Closed/Published
Development

No branches or pull requests

3 participants