Opened 2 years ago

Closed 19 months ago

#27396 closed enhancement (fixed)

add libsemigroups package

Reported by: dimpase Owned by:
Priority: major Milestone: sage-8.9
Component: packages: optional Keywords: GAP packages, semigroups
Cc: nthiery, isuruf Merged in:
Authors: Dima Pasechnik Reviewers: Travis Scrimshaw, Isuru Fernando
Report Upstream: N/A Work issues:
Branch: 300736d (Commits, GitHub, GitLab) Commit: 300736dba9324f872110fd0f91edb29eba3a9664
Dependencies: #28088 Stopgaps:

Status badges

Description (last modified by dimpase)

libsemigroups is a C++ library for computing in semigroups, and is used by semigroups GAP package, which is to be included in Sage, cf. #27295

So this automatically gives it rights to become an optional package.

tarball: http://users.ox.ac.uk/~coml0531/sage/libsemigroups-0.6.7.tar.gz

To the release manager, merge simultaneous with #27295.

Change History (25)

comment:1 Changed 2 years ago by dimpase

  • Authors set to Dima Pasechnik
  • Branch set to u/dimpase/packages/libsemigroup
  • Commit set to 15bf22f537c4f5bcec0a54e8efe9f758e304493e
  • Description modified (diff)
  • Status changed from new to needs_review

Ready for review. On another ticket we should also add its Python bindings: https://github.com/james-d-mitchell/libsemigroups-python-bindings/


New commits:

15bf22fadd libsemigroups package

comment:2 follow-up: Changed 2 years ago by tscrim

It installed and package tests were okay for me (Ubuntu 18.04 LTS). However, I think this is mostly useless package for Sage without the Python bindings (or GAP's semigroups package). So I think we should add the Python bindings as part of this ticket too.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 2 years ago by dimpase

Replying to tscrim:

It installed and package tests were okay for me (Ubuntu 18.04 LTS). However, I think this is mostly useless package for Sage without the Python bindings (or GAP's semigroups package).

I don't quite understand the point you are trying to make here. This ticket is a dependency of #27295, which is for exactly this, adding GAP's semigroups package.

So I think we should add the Python bindings as part of this ticket too.

No, not here. Adding python bindings should be on another ticket.

comment:4 in reply to: ↑ 3 Changed 2 years ago by tscrim

Replying to dimpase:

Replying to tscrim:

It installed and package tests were okay for me (Ubuntu 18.04 LTS). However, I think this is mostly useless package for Sage without the Python bindings (or GAP's semigroups package).

I don't quite understand the point you are trying to make here. This ticket is a dependency of #27295, which is for exactly this, adding GAP's semigroups package.

This does not make sense to me as a standalone (at least, it seems like the Cython code would be complicated to generated examples, but maybe I didn't look closely enough). Based on that, I feel either this should be as part of #27295 or have the Python bindings added to this ticket.

comment:5 Changed 2 years ago by dimpase

  • Dependencies set to #27295

Now it is a part of #27295.

comment:6 Changed 2 years ago by dimpase

A ticket should not be trying to do too much, and get too hard to review due to this.

comment:7 Changed 2 years ago by tscrim

I agree, but I also think tickets also should have some standalone meaning. Different commits can help granularize changes similar to splitting tickets. I just don't think this quite passes the standalone test.

comment:8 Changed 2 years ago by dimpase

We have added standalone C++ libraries without any bindings and a clear plan why they are needed in the past, cf e.g. mprfcx. This one is much better in this respect.

comment:9 Changed 2 years ago by dimpase

Different projects have different commits per ticket policies. We don't have any, and I seldom see people reviewing tickets per commit, either.

comment:10 Changed 2 years ago by tscrim

  • Description modified (diff)
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

I am not one to generally follow something I do not agree with just because there is prescient. I am only setting a positive review here under the condition that this is merged with #27295, but I feel it would be better for this to be included as part of #27295.

comment:11 Changed 2 years ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:12 Changed 20 months ago by chapoton

  • Milestone changed from sage-8.8 to sage-pending

comment:13 Changed 20 months ago by dimpase

  • Dependencies changed from #27295 to #27295, #28088
  • Milestone changed from sage-pending to sage-8.9

comment:14 Changed 20 months ago by embray

  • Description modified (diff)

Updated link in the description to the current source code home of libsemigroups.

comment:15 Changed 20 months ago by embray

  • Description modified (diff)

Oops, pasted the wrong link.

comment:16 Changed 20 months ago by git

  • Commit changed from 15bf22f537c4f5bcec0a54e8efe9f758e304493e to e1d5ab1b8b23eced4721964453249b478f76e64c
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. This was a forced push. New commits:

32aab07update to GAP 4.10.2
e1d5ab1add libsemigroups package

comment:17 Changed 20 months ago by dimpase

just a version bump and a rebase over the latest GAP

comment:18 Changed 20 months ago by dimpase

  • Status changed from needs_review to needs_work

this needs a modified tarball, as the ones available do not come with ./confugure, one needs autotools to get one.

comment:19 Changed 20 months ago by git

  • Commit changed from e1d5ab1b8b23eced4721964453249b478f76e64c to 300736dba9324f872110fd0f91edb29eba3a9664

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

300736dcorrect checksum for the tarball

comment:20 Changed 20 months ago by dimpase

  • Description modified (diff)
  • Status changed from needs_work to needs_review

I've linked the correct tarball, with ready ./configure. One needs to untar the original upstream tarball, which is bare, run ./autogen there (having autotools installed), and re-tar.

comment:21 Changed 20 months ago by dimpase

  • Cc isuruf added

comment:22 follow-up: Changed 20 months ago by isuruf

  • Dependencies changed from #27295, #28088 to #28088
  • Reviewers changed from Travis Scrimshaw to Travis Scrimshaw, Isuru Fernando
  • Status changed from needs_review to positive_review

comment:23 in reply to: ↑ 22 Changed 20 months ago by dimpase

  • Dependencies changed from #28088 to #28088, #27295

Replying to isuruf: Could you also review #27295 (this is adding GAP interface to this library, whjch was the original purpose of this ticket all along)

comment:24 Changed 19 months ago by isuruf

  • Dependencies changed from #28088, #27295 to #28088

comment:25 Changed 19 months ago by vbraun

  • Branch changed from u/dimpase/packages/libsemigroup to 300736dba9324f872110fd0f91edb29eba3a9664
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.