Skip to content

TYP: Annotations in pandas/core/nanops.py #30461

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 20 commits into from
Jan 20, 2020
Merged

TYP: Annotations in pandas/core/nanops.py #30461

merged 20 commits into from
Jan 20, 2020

Conversation

@ShaharNaveh
Copy link
Contributor

@ShaharNaveh ShaharNaveh commented Dec 25, 2019 •
edited
Loading

  • closes #xxxx
  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

@alimcmaster1 alimcmaster1 added the Typing type annotations, mypy/pyright type checking label Dec 25, 2019
jreback
jreback requested changes Dec 25, 2019
View reviewed changes
pandas/core/nanops.py Outdated
def nancorr(
a: np.ndarray,
b: np.ndarray,
method: str = "pearson",
Copy link
Contributor

@jreback jreback Dec 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be Union[str, Callable[...]]

Copy link
Contributor Author

@ShaharNaveh ShaharNaveh Dec 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mypy raises errors

Copy link
Contributor

@jreback jreback Dec 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then something else is wrong

@ShaharNaveh ShaharNaveh force-pushed the TYP-core-nanops branch 4 times, most recently from f12b581 to bce03ad Compare December 25, 2019 23:30
@jbrockmendel
Copy link
Member

jbrockmendel commented Dec 26, 2019

In the CI logs:

>           from scipy.stats import kendalltau, spearmanr
E           ModuleNotFoundError: No module named 'scipy'

For this to work you need to first do from scipy import stats, then do the specific imports. I think a recent change in the test imports may be the reason why this PR was affected, but the import here definitely needs to be updated.

@jbrockmendel
Copy link
Member

jbrockmendel commented Dec 27, 2019

apparently a test in test_nanops is missing a td.skip_if_no_scipy. have you rebased recently?

@ShaharNaveh
Copy link
Contributor Author

ShaharNaveh commented Dec 28, 2019

have you rebased recently?

Yes, I have

jbrockmendel
jbrockmendel reviewed Dec 28, 2019
View reviewed changes
jbrockmendel
jbrockmendel reviewed Dec 28, 2019
View reviewed changes
pandas/core/nanops.py Outdated
skipna: bool = True,
min_count: int = 0,
mask: Optional[np.ndarray] = None,
) -> Dtype:
Copy link
Member

@jbrockmendel jbrockmendel Dec 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dtype as the return type looks sketchy. are you sure about this?

jbrockmendel
jbrockmendel reviewed Dec 28, 2019
View reviewed changes
jbrockmendel
jbrockmendel reviewed Dec 28, 2019
View reviewed changes
pandas/core/nanops.py Outdated
if method in ["kendall", "spearman"]:
def get_corr_func(method) -> Callable:
if method in ["kendall", "spearman", "pearson"]:
from scipy import stats
Copy link
Member

@jbrockmendel jbrockmendel Dec 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the CI issue is that this should be import scipy.stats

@ShaharNaveh ShaharNaveh force-pushed the TYP-core-nanops branch 3 times, most recently from 3b1f0c7 to 0b6b26f Compare December 31, 2019 11:05
simonjayhawkins
simonjayhawkins reviewed Dec 31, 2019
View reviewed changes
pandas/core/nanops.py Outdated
Returns
-------
result : dtype
The product of all elements on a given axis. ( NaNs are treated as 1)
Copy link
Member

@simonjayhawkins simonjayhawkins Dec 31, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be type and then description. same as _maybe_null_out?

simonjayhawkins
simonjayhawkins reviewed Dec 31, 2019
View reviewed changes
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment

ShaharNaveh
ShaharNaveh commented Jan 1, 2020
View reviewed changes
@jreback
Copy link
Contributor

jreback commented Jan 1, 2020

looks ok, @MomIsBestFriend there is the return type of nanprod that is incorrect (and not erroring) ?

@ShaharNaveh
Copy link
Contributor Author

ShaharNaveh commented Jan 1, 2020 •
edited
Loading

looks ok, @MomIsBestFriend there is the return type of nanprod that is incorrect (and not erroring) ?

On my PC the type is

<class 'numpy.float64'>

I think that's is because my CPU is 64-bit architecture, should I annotate it as

Union[np.float32, np.float64]

to add support for 32-bit architecture CPUs as well?
or just to annotate it as float or np.float?

cc @jreback @simonjayhawkins

@ShaharNaveh ShaharNaveh mentioned this pull request Jan 1, 2020
Merged
5 tasks
@simonjayhawkins
Copy link
Member

simonjayhawkins commented Jan 2, 2020

or just to annotate it as float or np.float?

just float for now, since np.float resolves to Any without stubs for numpy.

@ShaharNaveh ShaharNaveh requested a review from jreback January 12, 2020 16:00
jreback
jreback requested changes Jan 18, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls rebase as well

pandas/core/nanops.py Outdated


def get_corr_func(method):
def get_corr_func(method) -> Callable:
Copy link
Contributor

@jreback jreback Jan 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method: Union[str, Callable]

Copy link
Contributor Author

@ShaharNaveh ShaharNaveh Jan 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 7442310 I removed all annotations of str and Callable that were added in this PR, it just gave me too much trouble.

MomIsBestFriend added 3 commits January 18, 2020 23:41
Merge remote-tracking branch 'upstream/master' into TYP-core-nanops
dfb02ab
Changed the name of "dt" to "dtype"
3283564 
Ref: pandas-dev#30461 (comment)
Removed any newly added annotation of Callable/str
7442310 
This is because no matter what typing annotation combination of "method"
is being used "Union[Callable, str]" or "Callable" or "str" mypy is
raising errors
@ShaharNaveh
Copy link
Contributor Author

ShaharNaveh commented Jan 18, 2020

pls rebase as well

Rebased.

@ShaharNaveh ShaharNaveh requested a review from jreback January 19, 2020 01:02
@jreback jreback added this to the 1.1 milestone Jan 20, 2020
@jreback
Copy link
Contributor

jreback commented Jan 20, 2020

lgtm. @simonjayhawkins or @WillAyd if any comments. let's merge if good rather than add things to this one. ideally we keep basic typing PRs non-controversial and separate out the controversial to other PRs.

@WillAyd WillAyd merged commit 264363e into pandas-dev:master Jan 20, 2020
@WillAyd
Copy link
Member

WillAyd commented Jan 20, 2020

Thanks @MomIsBestFriend

@ShaharNaveh ShaharNaveh deleted the TYP-core-nanops branch January 20, 2020 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@WillAyd WillAyd WillAyd approved these changes

@jreback jreback jreback approved these changes

@simonjayhawkins simonjayhawkins Awaiting requested review from simonjayhawkins

Assignees

No one assigned

Labels

Typing type annotations, mypy/pyright type checking

Projects

None yet

Milestone

1.1

Development

Successfully merging this pull request may close these issues.

6 participants

@ShaharNaveh @jbrockmendel @jreback @simonjayhawkins @WillAyd @alimcmaster1