REF: add custom Exception for safe_sort
State: closed

xref https://github.com/pandas-dev/pandas/pull/25537#issuecomment-469425015

custom `Exception` added primarily to allow more targeted testing, but also to distinguish sort failures and bad input to `safe_sort` 

Labels:
name: Error Reporting
description: Incorrect or improved errors from pandas

Comments:
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25569?src=pr&el=h1) Report
> Merging [#25569](https://codecov.io/gh/pandas-dev/pandas/pull/25569?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/96a128eaa0a7425dd4285d219780ef29c2727e46?src=pr&el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `76.92%`.

[![Impacted file tree graph](https://codecov.io/gh/pandas-dev/pandas/pull/25569/graphs/tree.svg?width=650&token=eZ4WkYLtcO&height=150&src=pr)](https://codecov.io/gh/pandas-dev/pandas/pull/25569?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #25569      +/-   ##
==========================================
- Coverage   91.77%   91.76%   -0.02%     
==========================================
  Files         175      175              
  Lines       52607    52624      +17     
==========================================
+ Hits        48282    48288       +6     
- Misses       4325     4336      +11
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.32% <76.92%> (-0.01%)` | :arrow_down: |
| #single | `41.9% <11.53%> (-0.08%)` | :arrow_down: |

| [Impacted Files](https://codecov.io/gh/pandas-dev/pandas/pull/25569?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pandas/core/sorting.py](https://codecov.io/gh/pandas-dev/pandas/pull/25569/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvc29ydGluZy5weQ==) | `98.32% <100%> (+0.02%)` | :arrow_up: |
| [pandas/core/reshape/merge.py](https://codecov.io/gh/pandas-dev/pandas/pull/25569/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvcmVzaGFwZS9tZXJnZS5weQ==) | `94.23% <50%> (-0.26%)` | :arrow_down: |
| [pandas/core/algorithms.py](https://codecov.io/gh/pandas-dev/pandas/pull/25569/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvYWxnb3JpdGhtcy5weQ==) | `94.52% <75%> (-0.26%)` | :arrow_down: |
| [pandas/core/indexes/base.py](https://codecov.io/gh/pandas-dev/pandas/pull/25569/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvaW5kZXhlcy9iYXNlLnB5) | `96.47% <75%> (-0.1%)` | :arrow_down: |
| [pandas/io/gbq.py](https://codecov.io/gh/pandas-dev/pandas/pull/25569/diff?src=pr&el=tree#diff-cGFuZGFzL2lvL2dicS5weQ==) | `75% <0%> (-12.5%)` | :arrow_down: |
| [pandas/core/frame.py](https://codecov.io/gh/pandas-dev/pandas/pull/25569/diff?src=pr&el=tree#diff-cGFuZGFzL2NvcmUvZnJhbWUucHk=) | `96.79% <0%> (-0.12%)` | :arrow_down: |
| [pandas/util/testing.py](https://codecov.io/gh/pandas-dev/pandas/pull/25569/diff?src=pr&el=tree#diff-cGFuZGFzL3V0aWwvdGVzdGluZy5weQ==) | `90.46% <0%> (-0.11%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25569?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/25569?src=pr&el=footer). Last update [96a128e...94d76fc](https://codecov.io/gh/pandas-dev/pandas/pull/25569?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

what is the reason for this change?
> what is the reason for this change?

during the process of converting the pytest.raises tests to use a context manager, two tests have so far had the match parameter added to check that the correct error is raised, but these were only testing safe_sort and factorise helper functions directly.

There are still many tests that require the match parameter added and there are several instances in the code base where the calls to safe-sort are not wrapped in a try/except block.

This PR is not critical at the moment and can be put on hold until more tests have been updated and further work on testing error messages is completed. 
 
@simonjayhawkins right not objecting to making testing easier. This exception is a good idea, though have to think of implications for a user level exception like this.
> though have to think of implications for a user level exception like this.

i'm assuming that will need to wrap all uses of safe_sort and then raise an appropriate user facing Exception or RunTimeWarning (I believe that @reidy-p has already started doing this). so the SortError is not intended to be user facing.
> i'm assuming that will need to wrap all uses of safe_sort and then raise an appropriate user facing Exception or RunTimeWarning (I believe that @reidy-p has already started doing this). so the SortError is not intended to be user facing.

yep exactly (though maybe we just make it an internal exception for this reason)
> though maybe we just make it an internal exception for this reason

do you mean define the Exception class in `pandas/core/sorting.py` instead of `pandas/errors/__init__.py`?
actually I kind of like this custom exception now.

cc @TomAugspurger @jorisvandenbossche 
I am personally not a fan of adding custom user-level exceptions if there is no good reason for it (and here it only seems for testing?) 

But is it the intent to expose it to users? The discussion above about it is not fully clear to me.
if you can merge master and update. I believe this would be ok if SortError is just an internal exception (e.g. define in pandas/core/sorting), mainly to disambiguate on testing; raising an appropriate error to users.
Note that the current version of the PR *does* expose it publicly (from a quick look at the diff in `pd.factorize`, and also in `pandas.errors`)
@jorisvandenbossche 

> Note that the current version of the PR does expose it publicly (from a quick look at the diff in pd.factorize, and also in pandas.errors)

right my suggestion was to make it internal
started to internalize the custom error, still need to wrap other calls to safe_sort.
i'm not sure that at the moment there is much to gain from the added complexity here.

and also there is the helper function `_is_unorderable_exception` (that I only found after opening this PR) which serves the purpose of distinguishing sort failures and bad input to `safe_sort`

regexes should probably just be simplified to `"not supported between instances|unorderable types"` for testing purposes.

@simonjayhawkins agree with your comment. ``_is_unorderable_exception`` basically does this, so closing.