REF: de-duplicate Categorical _validate_foo_value by jbrockmendel · Pull Request #41919 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation11 Commits12 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- closes #xxxx
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
- whatsnew entry
TypeError is "more correct" for these anyway, and we come within striking distance of sharing with datetimelike.
TypeError is "more correct" for these anyway
release note? I think we normally add when changing the exception type (but not always for just a change in message)
Can you also explain why TypeError is "more correct".
release note? I think we normally add when changing the exception type (but not always for just a change in message)
Just pushed with note
Can you also explain why TypeError is "more correct".
My reasoning is that TypeError makes more sense than ValueError here because it is determined solely by the dtype. By contrast, ValueError would make sense if we had a valid-for-the-dtype value that was an incompatible shape. KeyError is only being replaced in a few places, and it just doesn't make sense at all in these non-indexing/lookup contexts.
1 similar comment
release note? I think we normally add when changing the exception type (but not always for just a change in message)
Just pushed with note
Can you also explain why TypeError is "more correct".
My reasoning is that TypeError makes more sense than ValueError here because it is determined solely by the dtype. By contrast, ValueError would make sense if we had a valid-for-the-dtype value that was an incompatible shape. KeyError is only being replaced in a few places, and it just doesn't make sense at all in these non-indexing/lookup contexts.
@simonjayhawkins is it time to move whatsnew notes for existing PRs to the 1.4 file?
@simonjayhawkins is it time to move whatsnew notes for existing PRs to the 1.4 file?
not sure @jreback ?
I started that task after we branched but was asked not to milestone things. Without being able to add milestones doing that task seemed pointless so I stopped.
If a PR has a release note then we need to assess whether we will be backporting or ask the contributor to move the release note.
If a PR doesn't have a release note then moving release note is not applicable and unless it's CI related then we probably are not backporting to 1.3.x
This PR is titled as a refactor. but has a change in behavior so probably needs a change in title as well.
If a PR has a release note then we need to assess whether we will be backporting or ask the contributor to move the release note.
it's not just release notes, we also need to check PRs for versionadded and versionchanged tags.
let's target 1.4 with this
| @@ -903,7 +903,6 @@ Categorical |
|---|
| - Bug in constructing a :class:`DataFrame` from an ``ndarray`` and a :class:`CategoricalDtype` (:issue:`38857`) |
| - Bug in setting categorical values into an object-dtype column in a :class:`DataFrame` (:issue:`39136`) |
| - Bug in :meth:`DataFrame.reindex` was raising an ``IndexError`` when the new index contained duplicates and the old index was a :class:`CategoricalIndex` (:issue:`38906`) |
| - Bug in :meth:`Categorical.fillna` with a tuple-like category raising ``NotImplementedError`` instead of ``ValueError`` when filling with a non-category tuple (:issue:`41914`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should remain? #41914 is in 1.3 and is not backporting this PR is the correct description of behavior for 1.3
| - Bug in :meth:`Categorical.searchsorted` when passing a dtype-incompatible value raising ``KeyError`` instead of ``TypeError`` (:issue:`41919`) |
|---|
| - Bug in :meth:`Series.where` with ``CategoricalDtype`` when passing a dtype-incompatible value raising ``ValueError`` instead of ``TypeError`` (:issue:`41919`) |
| - Bug in :meth:`Categorical.fillna` when passing a dtype-incompatible value raising ``ValueError`` instead of ``TypeError`` (:issue:`41919`) |
| - Bug in :meth:`Categorical.fillna` with a tuple-like category raising ``NotImplementedError`` instead of ``TypeError`` when filling with a non-category tuple (:issue:`41914`) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Bug in :meth:`Categorical.fillna` with a tuple-like category raising ``NotImplementedError`` instead of ``TypeError`` when filling with a non-category tuple (:issue:`41914`) |
|---|
| - Bug in :meth:`Categorical.fillna` with a tuple-like category raising ``ValueError`` instead of ``TypeError`` when filling with a non-category tuple (:issue:`41914`) |
will have to re-loook can you rebase
feefladder pushed a commit to feefladder/pandas that referenced this pull request
