REF: de-duplicate Categorical _validate_foo_value #41919
Conversation
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". |
simonjayhawkins
added
Categorical
Just pushed with note
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
Just pushed with note
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? |
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. |
it's not just release notes, we also need to check PRs for versionadded and versionchanged tags. |
|
let's target 1.4 with this |
doc/source/whatsnew/v1.3.0.rst
Outdated
| @@ -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`) | |||
There was a problem hiding this comment.
This should remain? #41914 is in 1.3 and is not backporting this PR is the correct description of behavior for 1.3
doc/source/whatsnew/v1.4.0.rst
Outdated
| - 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`) |
There was a problem hiding this comment.
| - 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 |
|
thanks, makes sense |
TypeError is "more correct" for these anyway, and we come within striking distance of sharing with datetimelike.