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

REF/DEPR: Deprecate AbstractMethodError #48913

Closed
wants to merge 4 commits into from
Closed

REF/DEPR: Deprecate AbstractMethodError #48913

wants to merge 4 commits into from

Conversation

twoertwein
Copy link
Member

@twoertwein twoertwein commented Oct 2, 2022

closes #48909

Probably would need a whatsnew entry, for people inheriting from those now abstract classes. I'll keep this PR as a draft until I replaced all cases of AbstractMethodError (that are actually implemented).

@twoertwein twoertwein marked this pull request as draft October 3, 2022 03:00
@mroeschke mroeschke added Refactor Internal refactoring of code Error Reporting Incorrect or improved errors from pandas labels Oct 3, 2022
@mroeschke
Copy link
Member

Just a reminder to please run the ASVs just to ensure there is no perf hit as mentioned in the original issue

@twoertwein
Copy link
Member Author

I was able to avoid ASV until now :) Can I compare two ASV runs on python 3.10 (that's the only one I have installed):

$ asv continuous -f 1.1 -E virtualenv upstream/main HEAD
· No executable found for python 3.8
· No environments selected
$ asv continuous -f 1.1 -E existing upstream/main HEAD
· No range spec may be specified if benchmarking in an existing environment

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

This pull request is stale because it has been open for thirty days with no activity. Please update and respond to this comment if you're still interested in working on this.

@github-actions github-actions bot added the Stale label Nov 3, 2022
@mroeschke
Copy link
Member

Still interested in this @twoertwein?

@twoertwein twoertwein removed the Stale label Nov 17, 2022
@twoertwein twoertwein changed the title REF: convert a few AbstractMethodError-raising methods to abstractmethods REF/DEPR: Deprecate AbstractMethodError Nov 19, 2022
@twoertwein twoertwein marked this pull request as ready for review November 19, 2022 13:33
@twoertwein
Copy link
Member Author

Still interested in this @twoertwein?

Ready for review now :)

@twoertwein
Copy link
Member Author

twoertwein commented Dec 19, 2022

I finally setup python 3.8 and ran asv continuous -f 1.1 -E virtualenv upstream/main abstract. Stupid question, how do I know whether there were/were no significant changes? It says neither of it at the bottom of the output.

@mroeschke @jbrockmendel

@twoertwein
Copy link
Member Author

Happy to rebase occasionally - let me know if there is something else needed for this PR.

@twoertwein
Copy link
Member Author

Would be great to get this deprecation in for 2.0 @mroeschke

@mroeschke
Copy link
Member

It looks fairly good. Just noting that ideally we want to remove all the old 1.x deprecations before introducing new ones

@twoertwein
Copy link
Member Author

It looks fairly good. Just noting that ideally we want to remove all the old 1.x deprecations before introducing new ones

Are now all the 1.x deprecations removed? Rebased it earlier today.

@simonjayhawkins
Copy link
Member

Would be great to get this deprecation in for 2.0 @mroeschke

now that the rc is cut, aiming for 2.1?

@mroeschke
Copy link
Member

mroeschke commented Feb 22, 2023

Yeah 2.1 would be good. Sorry @twoertwein for not getting back to this for a bit.

@twoertwein
Copy link
Member Author

After rebasing mypy reported new errors:

pandas/core/series.py:1984: error: Cannot instantiate abstract class "SeriesGroupBy" with abstract attribute "_indexed_output_to_ndframe" [abstract]
pandas/core/groupby/groupby.py:4240: error: Can only assign concrete classes to a variable of type "Type[GroupBy[Any]]" [misc]
pandas/core/groupby/generic.py:1811: error: Cannot instantiate abstract class "SeriesGroupBy" with abstract attribute "_indexed_output_to_ndframe" [abstract]
pandas/core/groupby/generic.py:1862: error: Cannot instantiate abstract class "SeriesGroupBy" with abstract attribute "_indexed_output_to_ndframe" [abstract]

So I remove @abstractmethod from SeriesGroupBy._indexed_output_to_ndframe and instead raise inside the method.

@jbrockmendel
Copy link
Member

_indexed_output_to_ndframe is no longer needed for SGB, can be removed and only exist on DFGB

@twoertwein
Copy link
Member Author

The ASV Benchmark test seems to have failed - is there a summary of the 11k-line output?

@mroeschke
Copy link
Member

Thinking aloud, it might be best to do this apart of 3.0 as a "breaking change without deprecation

@mroeschke mroeschke added this to the 3.0 milestone Aug 1, 2023
@twoertwein
Copy link
Member Author

twoertwein commented Aug 4, 2023

I opened a new pull request #54408 to keep the commit history in this PR intact (rebasing a squashed PR is easier)

@twoertwein twoertwein closed this Aug 4, 2023
@twoertwein twoertwein deleted the abstract branch August 9, 2023 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Error Reporting Incorrect or improved errors from pandas Refactor Internal refactoring of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mark methods raising AbstractMethodError as abstractmethods?
4 participants