Opened 2 months ago

Closed 2 months ago

#33949 closed enhancement (fixed)

get rid of have_ring option in singular interface

Reported by: chapoton Owned by:
Priority: minor Milestone: sage-9.7
Component: commutative algebra Keywords:
Cc: Merged in:
Authors: Frédéric Chapoton Reviewers: Kwankyu Lee
Report Upstream: N/A Work issues:
Branch: 8dc327a (Commits, GitHub, GitLab) Commit: 8dc327a72bde1a0d20cd69fce80953bde13b7edc
Dependencies: Stopgaps:

Status badges

Description (last modified by klee)

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

Change History (14)

comment:1 Changed 2 months ago by chapoton

  • Branch set to u/chapoton/33949
  • Commit set to 8dc327a72bde1a0d20cd69fce80953bde13b7edc
  • Status changed from new to needs_review

New commits:

8dc327aget rid of "have_ring" in singular interface

comment:2 Changed 2 months ago by klee

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.

Last edited 2 months ago by klee (previous) (diff)

comment:3 Changed 2 months ago by chapoton

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):

comment:4 Changed 2 months ago by klee

#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.

comment:5 Changed 2 months ago by klee

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

comment:6 Changed 2 months ago by git

  • Commit changed from 8dc327a72bde1a0d20cd69fce80953bde13b7edc to 999e8efb080621727614fb95de02d1c4fcc3ac24

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

999e8efremove the comments about "being expensive"

comment:7 follow-up: Changed 2 months ago by chapoton

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.

comment:8 in reply to: ↑ 7 Changed 2 months ago by klee

Replying to chapoton:

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...

comment:9 Changed 2 months ago by klee

  • Reviewers set to Kwankyu Lee

comment:10 Changed 2 months ago by git

  • Commit changed from 999e8efb080621727614fb95de02d1c4fcc3ac24 to 8dc327a72bde1a0d20cd69fce80953bde13b7edc

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

comment:11 Changed 2 months ago by chapoton

Here is a new branch with the comments restored

comment:12 Changed 2 months ago by klee

  • Description modified (diff)
  • Priority changed from major to minor
  • Status changed from needs_review to positive_review

LGTM. Thanks.

comment:13 Changed 2 months ago by klee

  • Description modified (diff)

comment:14 Changed 2 months ago by vbraun

  • Branch changed from u/chapoton/33949 to 8dc327a72bde1a0d20cd69fce80953bde13b7edc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.