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

get rid of have_ring option in singular interface #33949

Closed
fchapoton opened this issue Jun 3, 2022 · 19 comments
Closed

get rid of have_ring option in singular interface #33949

fchapoton opened this issue Jun 3, 2022 · 19 comments

Comments

@fchapoton
Copy link
Contributor

We get rid of have_ring option in singular interfaces, which is already out of use, as suggested in #2331.

Component: commutative algebra

Author: Frédéric Chapoton

Branch/Commit: 8dc327a

Reviewer: Kwankyu Lee

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

@fchapoton
Copy link
Contributor Author

New commits:

8dc327aget rid of "have_ring" in singular interface

@fchapoton
Copy link
Contributor Author

Branch: u/chapoton/33949

@fchapoton
Copy link
Contributor Author

Commit: 8dc327a

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 5, 2022

comment:2

What is the rationale?

(1) Setting Singular ring is not expensive anymore.

(2) have_ring is not set True anywhere in Sage, and practically not used.

(3) Both.

@fchapoton
Copy link
Contributor Author

comment:3

I think the second point holds:

git grep -h "have_ring\s*=" src/
    def _singular_(self, singular=singular_default, have_ring=False, force=False):
        return self._singular_init_(singular, have_ring=have_ring)
    def _singular_init_(self, singular=singular_default, have_ring=False, force=False):
    def _singular_(self, singular=singular_default, have_ring=False):
    def _singular_(self, singular=singular, have_ring=False):
    def _singular_init_func(self, singular=singular, have_ring=False):
def _singular_func(self, singular=singular, have_ring=False):
#    return self._singular_init_(singular, have_ring=have_ring)
    return _singular_init_func(self, singular, have_ring=have_ring)
def _singular_init_func(self, singular=singular, have_ring=False):
    def _singular_(self, singular=singular_default, have_ring=False):
    def _singular_(self, G=None, have_ring=False):
    def _singular_init_(self, have_ring=False):

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 5, 2022

comment:4

#2331 seems to say that before libSingular, set_ring() was expensive, and have_ring option was useful, but after libSingular, set_ring() is a breeze, and have_ring is useless.

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 5, 2022

comment:5

Hence you can also delete the comment # this is expensive...

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

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

999e8efremove the comments about "being expensive"

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 5, 2022

Changed commit from 8dc327a to 999e8ef

@fchapoton
Copy link
Contributor Author

comment:7

I removed the comments, although I am not totally convinced that we use libsingular everywhere. There could be remaining things still using the singular pexpect interface, maybe.

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 6, 2022

comment:8

Replying to @fchapoton:

I removed the comments, although I am not totally convinced that we use libsingular everywhere.

Me neither.

There could be remaining things still using the singular pexpect interface, maybe.

It seems code like _singular_(singular).set_ring() is using the singular pexpect interface. It is really expensive. On my system, it takes about 100 microsecond. Hence those comments are still relevant. Sorry...

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 6, 2022

Reviewer: Kwankyu Lee

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2022

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jun 6, 2022

Changed commit from 999e8ef to 8dc327a

@fchapoton
Copy link
Contributor Author

comment:11

Here is a new branch with the comments restored

@kwankyu
Copy link
Collaborator

kwankyu commented Jun 7, 2022

comment:12

LGTM. Thanks.

@kwankyu

This comment has been minimized.

@kwankyu

This comment has been minimized.

@vbraun
Copy link
Member

vbraun commented Jun 12, 2022

Changed branch from u/chapoton/33949 to 8dc327a

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

3 participants