API: replace dropna=False option with na_sentinel=None in factorize
State: closed

- [x] closes #35667
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [x] whatsnew entry

Labels:
name: Testing
description: pandas testing functions or related to the test suite
name: Docs
name: API Design

Comments:
Hello @charlesdong1991! Thanks for updating this PR. We checked the lines you've touched for [PEP 8](https://www.python.org/dev/peps/pep-0008) issues, and found:











There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers: 

##### Comment last updated at 2020-09-02 14:12:23 UTC
emm, the failure seems irrelevant to me.
> emm, the failure seems irrelevant to me.

Test failure look quite related to https://github.com/pandas-dev/pandas/pull/35655

https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=40793&view=logs&j=404760ec-14d3-5d48-e580-13034792878f&t=f81e4cc8-d61a-5fb8-36be-36768e5c561a

```
______________ TestParquetPyArrow.test_s3_roundtrip_explicit_fs _______________
[gw2] win32 -- Python 3.8.5 C:\Miniconda\envs\pandas-dev\python.exe

self = <pandas.tests.io.test_parquet.TestParquetPyArrow object at 0x000002B24268BF10>
df_compat =    A    B
0  1  foo
1  2  foo
2  3  foo
s3_resource = s3.ServiceResource(), pa = 'pyarrow'
s3so = {'client_kwargs': {'endpoint_url': 'http://127.0.0.1:5555/'}}

    def test_s3_roundtrip_explicit_fs(self, df_compat, s3_resource, pa, s3so):
        s3fs = pytest.importorskip("s3fs")
        if LooseVersion(pyarrow.__version__) <= LooseVersion("0.17.0"):
            pytest.skip()
        s3 = s3fs.S3FileSystem(**s3so)
        kw = dict(filesystem=s3)
>       check_round_trip(
            df_compat,
            pa,
            path="pandas-test/pyarrow.parquet",
            read_kwargs=kw,
            write_kwargs=kw,
        )

pandas\tests\io\test_parquet.py:550:
```

cc @martindurant 
thanks @alimcmaster1 

ref to issue #35856 to address this.
lgtm @charlesdong1991 pls merge.
sure! @jorisvandenbossche  I will read it through and first comment on the issue later tonight.
i just took a look at @simonjayhawkins suggestion, actually, the example looks a bit irregular since `na_sentinel` should take in integers to represent "not found" cases, while boolean values happen to be converted to 1/0, so that it is working.


> since the keyword is not documented, could one assume that it is not yet part of the public API

I agree that `dropna` doesn't sound logical to `factorize` method and yet part of public API, but since `na_sentinel` is already public, isn't it kinda "API change" if we incorporate `na_sentinal` with `dropna` in which way we also allow users to assign `boolean` to `na_sentinel` (right now it is: `na_sentinel: int, default -1`) ? 

any thoughts? @jorisvandenbossche @simonjayhawkins 
Instead of `na_sentinel=False`, we could also use `na_sentinel=None` (to indicate: "do not use a na_sentinel"), which doesn't have any backward compatibility concerns (as it raised an error right now).
> Instead of `na_sentinel=False`, we could also use `na_sentinel=None` (to indicate: "do not use a na_sentinel"), which doesn't have any backward compatibility concerns (as it raised an error right now).

using `na_sentinel=None` results in `na_sentinel=lib.no_default` in the user-facing docs signature.

However, I'm also not a fan of boolean values as arguments unless both True/False have a meaning.

I'm not sure that `na_sentinel=True` makes much sense here, as currently coerces to 1 and then clashes with the generated factors.

so I agree that `na_sentinel=None` may be the better choice.
@jorisvandenbossche okay, I thought the idea was to only hide `dropna` for `series.factorize`

will change then.

@simonjayhawkins just a double check, you also meant to replace `dropna` with `na_sentinel=None` for algos.factorize, right?
`factorize` is a public top-level function (the discussion in the issue actually was about this `factorize` function, not the Series method), and I think we ideally keep the method consistent with the top-level function.
> @simonjayhawkins just a double check, you also meant to replace `dropna` with `na_sentinel=None` for algos.factorize, right?

yes, agree with @jorisvandenbossche  https://github.com/pandas-dev/pandas/pull/35852#issuecomment-682095068
@charlesdong1991 can you also update the title of the PR
@charlesdong1991 can ignore that 38_locale failure
thanks @jreback Will ignore and merge it!
@simonjayhawkins btw, i think should this be backported, right?
@meeseeksdev backport 1.1.x
> @simonjayhawkins btw, i think should this be backported, right?

yep. thanks. meeseeks... is only very occasionally picking these up now.
i've got a branch going for automating the release process, but in the first instance am using it for release readiness, i'm planning to add a check shortly that the whatsnew on the backport and master are the same. so this should make sure that any PRs that change the whatsnew aren't missed.

https://github.com/simonjayhawkins/pandas-release/actions?query=workflow%3A%22Manual+Release%22
> i'm planning to add a check shortly that the whatsnew on the backport and master are the same. so this should make sure that any PRs that change the whatsnew aren't missed.

That would be cool!