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

BUG: DatetimeArray._from_sequence accepting bool dtype #32668

Merged
merged 4 commits into from
Mar 15, 2020

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

Identified when making the change this makes in Series.

with pytest.raises(
ValueError, match="The dtype of 'values' is incorrect.*bool"
):
DatetimeArray(np.array([1, 2, 3], dtype="bool"))
DatetimeArray(arr)
Copy link
Member

Choose a reason for hiding this comment

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

why is this a ValueError when DatetimeArray._from_sequence(arr) is a TypeError? Is is also worth considering making error messages consistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

originally i made it type error, then changed for consistenty with... something, but i dont remember what. will try to figure that out and report bakc

@jreback jreback added Dtype Conversions Unexpected or buggy dtype conversions Datetime Datetime data dtype labels Mar 14, 2020
@jreback jreback added this to the 1.1 milestone Mar 14, 2020
@jreback
Copy link
Contributor

jreback commented Mar 14, 2020

looks fine. need a whatsnew as a bug fix for pd.to_datetime erroneously accepting bools

try:
arg, _ = maybe_convert_dtype(arg, copy=False)
except TypeError:
if errors == "coerce":
Copy link
Contributor

Choose a reason for hiding this comment

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

actually do we have coverage for all of these paths? i seem to recall we using similar code elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, we have tests in tests.tools.test_to_datetime that hit all of these cases

Copy link
Contributor

Choose a reason for hiding this comment

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

kk

also maybe able to de-duplicate some of this code

@jreback
Copy link
Contributor

jreback commented Mar 14, 2020

can you rebase your PRs again, now that azure if running. (likely not a big deal, but just to make sure)

@jreback jreback merged commit e7e5b61 into pandas-dev:master Mar 15, 2020
@jreback
Copy link
Contributor

jreback commented Mar 15, 2020

thanks

@jbrockmendel jbrockmendel deleted the dta-from-bool branch March 15, 2020 21:00
SeeminSyed pushed a commit to CSCD01-team01/pandas that referenced this pull request Mar 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Datetime Datetime data dtype Dtype Conversions Unexpected or buggy dtype conversions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants