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

Clean up some _element_constructor_() methods #29375

Closed
pjbruin opened this issue Mar 20, 2020 · 11 comments
Closed

Clean up some _element_constructor_() methods #29375

pjbruin opened this issue Mar 20, 2020 · 11 comments

Comments

@pjbruin
Copy link
Contributor

pjbruin commented Mar 20, 2020

The coercion framework never calls _element_constructor_() methods without optional or keyword arguments on an input that is already in the correct parent. This ticket removes some unnecessary checks for this case.

I checked that (the branches of) the if statements removed by this ticket are never called by temporarily inserting assert statements. In some other cases, I added doctests showing that the respective if statements cannot be removed (because the _element_constructor_() method has a optional or keyword arguments).

The deleted code in multi_polynomial_libsingular is no longer needed because these cases are handled by coercion maps from/via the base ring since #29247.

CC: @tscrim

Component: coercion

Author: Peter Bruin

Branch/Commit: b526945

Reviewer: Frédéric Chapoton, Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/29375

@pjbruin pjbruin added this to the sage-9.1 milestone Mar 20, 2020
@pjbruin
Copy link
Contributor Author

pjbruin commented Mar 20, 2020

Commit: 586f977

@pjbruin
Copy link
Contributor Author

pjbruin commented Mar 20, 2020

@fchapoton
Copy link
Contributor

comment:2

would you please fix the pyflakes warning from patchbot report ?

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 20, 2020

Branch pushed to git repo; I updated commit sha1. New commits:

b526945Trac 29375: remove unused imports

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 20, 2020

Changed commit from 586f977 to b526945

@pjbruin
Copy link
Contributor Author

pjbruin commented Mar 20, 2020

comment:4

Replying to @fchapoton:

would you please fix the pyflakes warning from patchbot report ?

Done, thanks.

@fchapoton
Copy link
Contributor

comment:5

looks good to me, and the bot is fully green.

Maybe Travis, who knows better the coercion framework, could give his word ?

@tscrim
Copy link
Collaborator

tscrim commented Mar 20, 2020

comment:6

Replying to @fchapoton:

Maybe Travis, who knows better the coercion framework, could give his word ?

Yes, what is in the ticket description is correct (it is faster to do it there because it is Cython too). So +1 for doing this.

@fchapoton
Copy link
Contributor

comment:7

ok, then let it be.

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton, Travis Scrimshaw

@vbraun
Copy link
Member

vbraun commented Mar 25, 2020

Changed branch from u/pbruin/29375-clean_up_element_constructor to b526945

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

No branches or pull requests

4 participants