-
-
Notifications
You must be signed in to change notification settings - Fork 316
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
Bugfix/763: DataFrameModel validate returns the correct type #1450
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Chad Hawkins <[email protected]>
Signed-off-by: Chad Hawkins <[email protected]>
Signed-off-by: Chad Hawkins <[email protected]>
Signed-off-by: Chad Hawkins <[email protected]>
Hi @chadwhawkins, so unfortunately this will introduce another issue, which is that these methods can also return modin, pyspark.pandas, and dask dataframes. The current typing is also incorrect, but we actually do need to do type overloading here to cover those other dataframe types. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1450 +/- ##
==========================================
- Coverage 94.28% 94.28% -0.01%
==========================================
Files 91 91
Lines 7013 7011 -2
==========================================
- Hits 6612 6610 -2
Misses 401 401 ☔ View full report in Codecov by Sentry. |
Ah ok, thanks @cosmicBboy. I'll continue iterating on this PR. |
Thanks @chadwhawkins let me know when you need a review on this |
@chadwhawkins you can rebase on |
Thanks @cosmicBboy, planning to revisit this soon |
Updates the return type casting of various model methods to return the correct type (
DataFrame
instead ofDataFrameBase
). The issue called for typing overload but that didn't seem to be necessary in looking at the code, because as best as I can tell calling these methods should always return the same thing (andDataFrame
inherits fromDataFrameBase
anyway).Resolves #763