BUG: Dense ranking with percent now uses 100% basis
State: closed

- `DataFrame.rank()` and `Series.rank()` when `method='dense'` and
  `pct=True` now scales to 100%.

See #15630

 - [ ] closes #15630 
 - [ ] tests added / passed
 - [X] passes ``git diff upstream/master | flake8 --diff``
 - [X] whatsnew entry


Labels:
name: Bug
name: Numeric Operations
description: Arithmetic, Comparison, and Logical operations

Comments:
FYI #15640 (can be done after this one). Or before if you are so inclined.
# [Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15639?src=pr&el=h1) Report
> Merging [#15639](https://codecov.io/gh/pandas-dev/pandas/pull/15639?src=pr&el=desc) into [master](https://codecov.io/gh/pandas-dev/pandas/commit/b6691127523f965003dbf877a358c81af5012989?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

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

```diff
@@           Coverage Diff           @@
##           master   #15639   +/-   ##
=======================================
  Coverage   91.69%   91.69%           
=======================================
  Files         150      150           
  Lines       49112    49112           
=======================================
  Hits        45035    45035           
  Misses       4077     4077
```

| Flag | Coverage Δ | |
|---|---|---|
| #multiple | `90.08% <ø> (ø)` | :arrow_up: |
| #single | `41.86% <ø> (ø)` | :arrow_up: |


------

[Continue to review full report at Codecov](https://codecov.io/gh/pandas-dev/pandas/pull/15639?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/15639?src=pr&el=footer). Last update [b669112...776ea60](https://codecov.io/gh/pandas-dev/pandas/pull/15639?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

Thanks, @jreback. Agreed that tests should be split out a bit and we should be testing other ranking `method` too. May take a bit to work through but happy to tackle as much as I can.
@rouzazari great. as I said, ok to leave tests where they are and split after / or do independent PR to split before (and rebase this one).
ok, pls rebase (after https://github.com/pandas-dev/pandas/issues/15640) and we can see where this is
can you update
Yes. Been delayed but hoping to complete this week. Will ping when I have updated.
np, thanks.
I rebased and parametrized the `Series` test, but I am still researching `method='min'` and others.

Also, we will still need a sub-section in `whatsnew` to show before/after examples
can you update
Yes. I've been a bit delayed, but working on a solution over the next couple days. Thanks for the comments above. I'll ping soon.
I have rebased and added tests for all ``method``s with ``pct=True``. 

Please note that tests for all other methods assumes the existing calculation is working as intended. This bug fix only changes ``method=dense``; however, this is a good starting point should we decide to change ``pct=True`` overall.

I'd like to leave this as a bug fix in the docs for now until we fully consider the overall idea of ``pct=True`` as discussed in #15630. After we have an understanding of whether ``pct=True`` is always based on 100% or if this only applies to ``method=dense``, then we can add a sub-section on how ``pct=True`` is changing (if at all).

Please let me know your thoughts/comments, if any.

Will ping when green. Thanks
@dsm054 can you have a look here.
@rouzazari : Looks like you have a merge conflict in the `whatsnew`.

@jreback , @dsm054 : What are your thoughts on this one?  As it got approved already, and no comments have been made for nearly two months, I would think this is good to go.
this looks pretty reasonable. I would expand the whatsnew to show how this is different than before (as its a numerical change, a more complete entry highlites it a bit more)
@rouzazari : Would you be able to address @jreback 's suggestion for the `whatsnew` ?
In the original ticket, we never got around to agreeing that we should fix other affected methods as well.  I haven't had my morning coffee yet so I could easily be missing something, but it looks like we're actually now testing that the behaviour of the others is _wrong_ in the same way that dense was wrong before this.

This work is definitely important enough to do what's needed to get into v0.21.0 IMHO.  Whether we deal with the other cases in this PR as well or open up another PR to handle it doesn't matter to me, but I think those should go in as well before the next release.
Glad to see this come back to life. Thanks for comments all.

@dsm054 is absolutely correct that the new tests for ranking methods (other than dense ranking) are testing the expected results given the existing code. I am not sure what the correct results are for each ranking method, but I wanted to make sure we have tests for when we fix the ranking methods. 

If you can help me determine the correct results for each ranking method, then I can update the pull request, or we can close as is and fix in a separate pull request.

@gfyoung: I'm happy to add jreback's whatsnew suggestion. Will update soon.

@rouzazari why don't you mark those 'suspect' ones as xfails, then we can address them one-by-one (in followups).
can you rebase and add xfails for those suspect ones
can you rebase
can you rebase
can you rebase
Yes, sorry for the delay. I will rebase and add xfails in the coming week.
thanks @rouzazari 
closing as stale. ping if you'd like to continue.
@jreback : I'd like to continue this 😄 
@jreback : PTAL.  Was there anything that was needed before merging?
@jreback : Friendly ping.
Travis failure is also on <a href="https://travis-ci.org/pandas-dev/pandas/jobs/350554653">master</a>.  Seems to be happening on <a href="https://circleci.com/gh/pandas-dev/pandas/11139#tests/containers/2">Circle</a> as well.