Skip to content
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.

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

gh:552 - Corrected Series.mean return type #622

Merged
merged 4 commits into from
Apr 10, 2023
Merged

gh:552 - Corrected Series.mean return type #622

merged 4 commits into from
Apr 10, 2023

Conversation

ramvikrams
Copy link
Contributor

@ramvikrams
Copy link
Contributor Author

ramvikrams commented Apr 3, 2023

Now MyPy will fail because of tests in files test_windowing,test_resampler,test_frame fail. So I think either we can suppress the warning or changing the DataFrame.mean() method

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

  • For the errors in test_windowing.py, change the return types of _mean() to np.float64
  • For the errors in test_resampler.py, change the return types of the functions h() in line 161 and f() in line 251 to np.float.64
  • For the errors in test_frame.py, change the return types of these functions to np.float64:
    • sum_mean() in line 2046
    • lfunc should have type Callable[[pd.DataFrame], np.float64] in line 2050

f2: float = s.mean(skipna=False)
f3: float = s.mean(numeric_only=False)
check(assert_type(s.mean(), np.float64), np.float64)
s1 = s.groupby(level=0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't change this test. please revert

check(assert_type(s.mean(), np.float64), np.float64)
s1 = s.groupby(level=0)
check(assert_type(s1.mean(skipna=False), pd.Series), np.float64)
check(assert_type(s1.mean(numeric_only=False), pd.Series), np.float64)
Copy link
Collaborator

Choose a reason for hiding this comment

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

above two tests should be using s.mean() and checking result against np.float64

@ramvikrams
Copy link
Contributor Author

ramvikrams commented Apr 4, 2023

  • For the errors in test_windowing.py, change the return types of _mean() to np.float64
  • For the errors in test_resampler.py, change the return types of the functions h() in line 161 and f() in line 251 to np.float.64
  • For the errors in test_frame.py, change the return types of these functions to np.float64:

    • sum_mean() in line 2046
    • lfunc should have type Callable[[pd.DataFrame], np.float64] in line 2050
  • for test_resampler.py
    Yes, I had done that on my local system aready but the problem is after doing this change, in h() has incompatible types and then same in f()
  • for test_frame
    Same for both of these 2 also they had incompatible types

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Apr 4, 2023

  • For the errors in test_windowing.py, change the return types of _mean() to np.float64
  • For the errors in test_resampler.py, change the return types of the functions h() in line 161 and f() in line 251 to np.float.64
  • For the errors in test_frame.py, change the return types of these functions to np.float64:

    • sum_mean() in line 2046
    • lfunc should have type Callable[[pd.DataFrame], np.float64] in line 2050
  • for test_resampler.py
    Yes, I had done that on my local system aready but the problem is after doing this change, in h() has incompatible types and then same in f()
  • for test_frame
    Same for both of these 2 also they had incompatible types

You may have to make changes to other parts of the tests to make this work, since the tests assumed float was the result of Series.mean(), and now you are changing it to np.float64. Feel free to do that, and I will review.

@ramvikrams
Copy link
Contributor Author

You may have to make changes to other parts of the tests to make this work, since the tests assumed float was the result of Series.mean(), and now you are changing it to np.float64. Feel free to do that, and I will review.

Thanks

@ramvikrams
Copy link
Contributor Author

Sir I actually what should we write the assert_type when it is unknown. Means the assert_type is Unknown itself.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Apr 6, 2023

Sir I actually what should we write the assert_type when it is unknown. Means the assert_type is Unknown itself.

If the results are coming back as Unknown from the type checker, it means there is a bug somewhere in the stubs that you'll have to figure out.

@ramvikrams
Copy link
Contributor Author

If the results are coming back as Unknown from the type checker, it means there is a bug somewhere in the stubs that you'll have to figure out.

thanks i'll do it

@ramvikrams
Copy link
Contributor Author

The test_resampler errors are now gone but could you suggest a fix for test_frame I could not get it

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Apr 9, 2023

The test_resampler errors are now gone but could you suggest a fix for test_frame I could not get it

Can you try the following?

  • Add np.integer | np.float to the type IndexIterScalar in _typing.pyi .

If that creates new typing errors, then change the callables for groupby.apply() that declare Scalar as a return type to also allow np.integer | np.float as a return type.

@ramvikrams
Copy link
Contributor Author

Done Sir

pandas-stubs/core/resample.pyi Outdated Show resolved Hide resolved
Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

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

thanks @ramvikrams

@Dr-Irv Dr-Irv merged commit c03e23c into pandas-dev:main Apr 10, 2023
13 checks passed
@ramvikrams ramvikrams deleted the t120 branch April 10, 2023 17:27
Dr-Irv pushed a commit that referenced this pull request Apr 13, 2023
Revert "gh:552 - Corrected Series.mean return type (#622)"

This reverts commit c03e23c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Series.mean() does not return an float
2 participants