Opened 22 months ago

Last modified 6 days ago

#30787 positive_review enhancement

package modular_resolution: Split out from p_group_cohomology

Reported by: mkoeppe Owned by:
Priority: critical Milestone: sage-9.7
Component: packages: optional Keywords: sd111
Cc: SimonKing, dimpase, jhpalmieri, tscrim, klee Merged in:
Authors: Matthias Koeppe, Dima Pasechnik, John Palmieri Reviewers: John Palmieri, Matthias Koeppe, Dima Pasechnik, Travis Scrimshaw
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: u/tscrim/modular_resolution_split-30787 (Commits, GitHub, GitLab) Commit: c7108e9a8f0ccf4fbc19f6cbfa799b5d6f347439
Dependencies: #34221, #34291 Stopgaps:

Status badges

Description (last modified by mkoeppe)

As discussed in https://trac.sagemath.org/ticket/28711#comment:64:

We split the current p_group_cohomology spkg:

In this ticket, we create the package modular_resolution, depending on meataxe-the-C-library, that installs:

  • a standalone C library libmodres.so in $SAGE_LOCAL/lib
  • some executables in $SAGE_LOCAL/bin, linked against libmodres
  • database of cohomology rings

This also fixes the build problems of p_group_cohomology.

Fixing the doctest failures of p_group_cohomology is outside of the scope of this ticket. That's #34333.

New upstream:

Attachments (3)

p_group_cohomology.patch (892 bytes) - added by culler 10 months ago.
patch for p_group_cohomology/spkg-install.in
include_dir.patch (1.4 KB) - added by culler 10 months ago.
patch to install in p_group_cohomology/patches
p_gp_coho_3_3_3p1_failures.txt (25.0 KB) - added by tscrim 7 days ago.
Current failures of 3.3.3p1 version

Download all attachments as: .zip

Change History (156)

comment:1 Changed 22 months ago by mkoeppe

  • Dependencies set to #28711

comment:2 Changed 22 months ago by SimonKing

I'll start with summing up what the current p_group_cohomology spkg comprises and how the parts depend upon each other.

  1. the C library libmodres, which also links against libmtx and thus depends on the meataxe spkg, but not necessarily on sage-meataxe.
  2. some executables linked against libmodres
  3. some module for Singular, that should be installed in SAGE_LOCAL/share/singular/LIB/ (that's where Singular expect modules)
  4. some package for GAP, which by #28711 should be installed in SAGE_LOCAL/share/gap/pkg/ (that's where GAP expect packages)
  5. some database of cohomology rings, to be installed in SAGE_LOCAL/share/pGroupCohomology (could in principle be moved to somewhere else, but is data and thus shouldn't be in places like SAGE_SRC)
  6. some pip installable modules linked against libmodres and libmtx and also depending on sage.matrix.matrix_gfpn_dense being built and uses some Cython headers from sage.structure. The pip installable modules could in principle be put into the Sage src tree, analogously to sage-meataxe. 2.-5. are run time dependencies.

comment:3 follow-up: Changed 22 months ago by SimonKing

Obviously, 1. and 2. from the previous comment should be in the new modular_resolution spkg. But at what point should 3.-5. be installed? Currently, 5. is installed when making modular_resolution, whereas 3.-4. are installed by p_group_cohomology's spkg-install.

Would it be better to install ALL of 3.-5. by modular_resolution's spkg-install (not by the make file)? Or ALL of 3.-5. by p_group_cohomology's spkg-install? Or something else?

comment:4 follow-up: Changed 22 months ago by dimpase

The python/cython modules should be left alone, do not bundle anything with it please.

Have you thought about upstreaming the Singular library?

comment:5 in reply to: ↑ 4 ; follow-up: Changed 22 months ago by SimonKing

Replying to dimpase:

The python/cython modules should be left alone, do not bundle anything with it please.

Have you thought about upstreaming the Singular library?

No. I'm not sure if I should. It is about the computation of Dickson invariants, though. Since Singular has some capabilities of computing non-modular and modular invariant rings of finite group actions, it would somehow make sense to include it in Singular. (EDIT: I think I'll ask the Singular developers).

Concerning the analogous question on the GAP package: In this case I think it makes no sense to make it an upstream package. It calls the executables of modular_resolution, writing initial data needed for p_group_cohomology into several files.

Last edited 22 months ago by SimonKing (previous) (diff)

comment:6 in reply to: ↑ 5 ; follow-up: Changed 22 months ago by dimpase

Replying to SimonKing:

Concerning the analogous question on the GAP package: In this case I think it makes no sense to make it an upstream package. It calls the executables of modular_resolution, writing initial data needed for p_group_cohomology into several files.

An important consideration is the ability to have spkg-check - e.g. for instance you can bundle the GAP package with modular_resolution, so that you can easily write some tests (in GAP) for all of them.

comment:7 in reply to: ↑ 6 Changed 22 months ago by SimonKing

Replying to dimpase:

An important consideration is the ability to have spkg-check - e.g. for instance you can bundle the GAP package with modular_resolution, so that you can easily write some tests (in GAP) for all of them.

Good point!

If one bundles modular_resolution together with the cohomology database (which is currently the case) and with the GAP package, one could add the following self test: Call a function from the GAP package that uses an executable from modular_resolution to create initial data for, say, the cohomology ring of the dihedral group, which is part of the database, i.e., one can verify that the newly created initial data coincides what is found in the data base.

comment:8 in reply to: ↑ 3 ; follow-up: Changed 22 months ago by mkoeppe

Replying to SimonKing:

Obviously, 1. and 2. from the previous comment should be in the new modular_resolution spkg. But at what point should 3.-5. be installed? Currently, 5. is installed when making modular_resolution, whereas 3.-4. are installed by p_group_cohomology's spkg-install.

Would it be better to install ALL of 3.-5. by modular_resolution's spkg-install (not by the make file)? Or ALL of 3.-5. by p_group_cohomology's spkg-install? Or something else?

parts 3.-4. are outside of the scope of this ticket.

part 5. Is it used by 1.-2.? Then it should be included here; otherwise, outside of the scope of this ticket.

comment:9 in reply to: ↑ 8 ; follow-up: Changed 22 months ago by SimonKing

Replying to mkoeppe:

parts 3.-4. are outside of the scope of this ticket.

You argued in #28711, comment 58, that "an spkg should either be a pip-installable Python package, or it should be a non-Python package (which can install anywhere in SAGE_LOCAL)."

If I understand correctly, it means that not only the C library libmodres but also the database as well as the helpers for GAP and Singular shouldn't be installed by p_group_cohomology (whose main part IS pip-installable). So, 3.-5. should be split out as well and thus ARE in the scope of this ticket.

part 5. Is it used by 1.-2.? Then it should be included here; otherwise, outside of the scope of this ticket.

Then tell me by what source the database should be provided, if it shouldn't be p_group_cohomology.

Last edited 22 months ago by SimonKing (previous) (diff)

comment:10 in reply to: ↑ 9 ; follow-up: Changed 22 months ago by mkoeppe

Replying to SimonKing:

Replying to mkoeppe:

parts 3.-4. are outside of the scope of this ticket.

You argued in #28711, comment 58, that "an spkg should either be a pip-installable Python package, or it should be a non-Python package (which can install anywhere in SAGE_LOCAL)."

If I understand correctly, it means that not only the C library libmodres but also the database as well as the helpers for GAP and Singular shouldn't be installed by p_group_cohomology (whose main part IS pip-installable).

Yes, but this ticket is titled "package modular_resolution: Split out from p_group_cohomology", so it is more narrowly focused than a ticket titled "Split p_group_cohomology into its irreducible components".

comment:11 in reply to: ↑ 10 ; follow-up: Changed 22 months ago by SimonKing

Replying to mkoeppe:

Yes, but this ticket is titled "package modular_resolution: Split out from p_group_cohomology", so it is more narrowly focused than a ticket titled "Split p_group_cohomology into its irreducible components".

I don't understand what you suggest.

Do you suggest to change the title and description of the ticket? Or do you suggest to keep the Singular and Gap helpers in p_group_cohomology and additionally to move the database from modular_cohomology (that's where the database currently is) to p_group_cohomology, even though it is basically a pip installable module? Or something else?

comment:12 in reply to: ↑ 11 Changed 22 months ago by mkoeppe

Replying to SimonKing:

Or do you suggest to keep the Singular and Gap helpers in p_group_cohomology

Yes, that's my suggestion for this ticket - to keep the ticket simple. Follow-up tickets can take care of further restructuring.

additionally to move the database from modular_cohomology (that's where the database currently is) to p_group_cohomology

If the modular_cohomology Makefile currently installs this database, there's probably no need to change it.

comment:13 follow-up: Changed 22 months ago by mkoeppe

If the database is part of the modular_resolution package, I would suggest to install an executable script (perhaps modres-config) that reports the install location.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 22 months ago by SimonKing

Replying to mkoeppe:

If the database is part of the modular_resolution package, I would suggest to install an executable script (perhaps modres-config) that reports the install location.

How can this be done? I mean, I know where the database ends up. Makefile.am is

ACLOCAL_AMFLAGS = -I m4
SUBDIRS = src

dbdir           = $(datadir)/pGroupCohomology
dist_db_DATA    = group_cohomology_database.tar

install-data-hook:
	cd $(DESTDIR)$(dbdir) && tar xf $(dist_db_DATA) && rm $(dist_db_DATA)

uninstall-hook:
	rm -r $(DESTDIR)$(dbdir)

and so the database will be in $SAGE_LOCAL/share/pGroupCohomology. But I guess a script returning that path (dependent on the value of SAGE_LOCAL is not what you suggest.

Is there a standard script in the autotools ecosystem that would provide the correct value of $(dbdir) both during and after (staged) installation?

comment:15 in reply to: ↑ 14 ; follow-up: Changed 22 months ago by mkoeppe

Replying to SimonKing:

Replying to mkoeppe:

If the database is part of the modular_resolution package, I would suggest to install an executable script (perhaps modres-config) that reports the install location.

How can this be done? I mean, I know where the database ends up. Makefile.am is

ACLOCAL_AMFLAGS = -I m4
SUBDIRS = src

dbdir           = $(datadir)/pGroupCohomology
dist_db_DATA    = group_cohomology_database.tar

install-data-hook:
	cd $(DESTDIR)$(dbdir) && tar xf $(dist_db_DATA) && rm $(dist_db_DATA)

uninstall-hook:
	rm -r $(DESTDIR)$(dbdir)

and so the database will be in $SAGE_LOCAL/share/pGroupCohomology. But I guess a script returning that path (dependent on the value of SAGE_LOCAL is not what you suggest.

Not depending on SAGE_LOCAL - the upstream package should make sense without Sage. Rather, depending on the configured install prefix.

Like this: Create template file modres-config.in

#! /bin/sh
echo "@datadir@/pGroupCohomology"

and add to configure.ac:

AC_CONFIG_FILES([modres-config], [chmod +x modres-config])

Is there a standard script in the autotools ecosystem that would provide the correct

value of $(dbdir) both during and after (staged) installation?

No, one would not want to read from DESTDIR.

comment:16 in reply to: ↑ 15 ; follow-up: Changed 22 months ago by SimonKing

Replying to mkoeppe:

Like this: Create template file modres-config.in

#! /bin/sh
echo "@datadir@/pGroupCohomology"

and add to configure.ac:

AC_CONFIG_FILES([modres-config], [chmod +x modres-config])

Is there a standard script in the autotools ecosystem that would provide the correct

value of $(dbdir) both during and after (staged) installation?

No, one would not want to read from DESTDIR.

Do I understand correctly that the syntax @datadir@ in modres-config.in makes it so that the value $(datadir) is inserted during creation of the modres-config script, so that calling it later provides the final destination of the database? Well, I simply didn't know how to achieve this and is what I meant by a "standard script in the autotools ecosystem".

To be clear: Do you suggest that this script will be called by the Python/Cython? modules to find out the location of the database?

comment:17 in reply to: ↑ 16 ; follow-up: Changed 22 months ago by mkoeppe

Replying to SimonKing:

Do I understand correctly that the syntax @datadir@ in modres-config.in makes it so that the value $(datadir) is inserted during creation of the modres-config script, so that calling it later provides the final destination of the database?

That's right. ./configure will create the file modres-config. There are, of course, many other ways to do it, like generating this simple script in an install-hook.

Well, I simply didn't know how to achieve this and is what I meant by a "standard script in the autotools ecosystem".

Ok, great; but note that it does not do anything about DESTDIR (and should not).

To be clear: Do you suggest that this script will be called by the Python/Cython? modules to find out the location of the database?

Yes.

comment:18 in reply to: ↑ 17 ; follow-up: Changed 22 months ago by SimonKing

Replying to mkoeppe:

To be clear: Do you suggest that this script will be called by the Python/Cython? modules to find out the location of the database?

Yes.

Is this ticket only about creating a new modular_resolution spkg, or also to modify p_group_cohomology to use the new split-off spkg? Or shall the latter be done on a new ticket?

While we are at it: Is there a recommended way (at least recommended in Sage) to read from the output of system calls in Python? os.popen, or subprocess.Popen or something else?

comment:19 in reply to: ↑ 18 Changed 22 months ago by mkoeppe

Replying to SimonKing:

Is this ticket only about creating a new modular_resolution spkg, or also to modify p_group_cohomology to use the new split-off spkg? Or shall the latter be done on a new ticket?

Both

While we are at it: Is there a recommended way (at least recommended in Sage) to read from the output of system calls in Python? os.popen, or subprocess.Popen or something else?

I just learned the hard way how to write this portably for Python >= 3.6 (all our currently supported Python versions): Take a look at the most recent version (after #30758) of src/sage/features/__init.py__ (package_systems).

        from subprocess import run, CalledProcessError, PIPE

        try:
            proc = run('sage-guess-package-system', shell=True, stdout=PIPE, stderr=PIPE, universal_newlines=True, check=True)
            _cache_package_systems = [PackageSystem(proc.stdout.strip())]
        except CalledProcessError:
            pass
Last edited 22 months ago by mkoeppe (previous) (diff)

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

Why would one want to create a custom script modres-config instead of hooking this up on pkg-config ? (I can show you the very easy autoconf/automake magic for this, I've done this for many projects used by Sage)

Then the Python interface is already there (with Python module pkgconfig - see e.g. src/sage/env.py)

comment:21 in reply to: ↑ 20 ; follow-up: Changed 22 months ago by SimonKing

Replying to dimpase:

Why would one want to create a custom script modres-config instead of hooking this up on pkg-config ?

I have no preference.

(I can show you the very easy autoconf/automake magic for this, I've done this for many projects used by Sage)

Please do!

comment:22 Changed 22 months ago by mkoeppe

We were discussing how to advertise the install location of the data files. Pkgconfig for the library would also be nice of course

comment:23 in reply to: ↑ 21 ; follow-up: Changed 22 months ago by SimonKing

Replying to SimonKing:

(I can show you the very easy autoconf/automake magic for this, I've done this for many projects used by Sage)

Please do!

In particular, I have no idea how to make pkgconfig aware of an autotoolized library: pkgconfig.list_all() contains neither libmtx nor mtx nor meataxe nor modres nor libmodres nor modular_resolution, although meataxe and modular_resolution are installed.

comment:24 in reply to: ↑ 23 ; follow-ups: Changed 22 months ago by dimpase

Replying to SimonKing:

Replying to SimonKing:

(I can show you the very easy autoconf/automake magic for this, I've done this for many projects used by Sage)

Please do!

In particular, I have no idea how to make pkgconfig aware of an autotoolized library: pkgconfig.list_all() contains neither libmtx nor mtx nor meataxe nor modres nor libmodres nor modular_resolution, although meataxe and modular_resolution are installed.

As an example, have a look at e.g. https://github.com/cddlib/cddlib/pull/40 (It changes a bit more than just adds cddlib.pc (adds some GMP-related stuff), so skip these parts) Namely,

  1. Makefile.am needed
    pkgconfigdir       = $(libdir)/pkgconfig
    pkgconfig_DATA     = cddlib.pc
    
  1. cddlib.pc.in (a template) needed to be created
  1. cddlib.pc had to be added to the list in AC_CONFIG_FILES

After all this, ./configure will fill in cddlib.pc.in and create cddlib.pc, which is then installed by make install. Then Python pkgconfig module will be aware of that .pc file, no problem.

By the way, you can have a custom variable in .pc file - in your case, say, the path to the database - and request it using pkg-config --variable=... ... calls (and from Python pkgconfig in a similar way)

comment:25 in reply to: ↑ 24 ; follow-up: Changed 22 months ago by SimonKing

Thank you, Dima!

I wonder: Should I add pkg-config to SharedMeatAxe as well? After all, meataxe uses arithmetic tables that are of course stored in some default folder, and currently its location is presented by an environment variable. This variable is needed by the meataxe executables (that are also part of the package), whereas libmtx uses a C variable for the same purpose.

I took this design from the original MeatAxe from which I forked off SharedMeatAxe, but I've never been happy with it.

On the other hand, I'm not sure if pkg-config would be the right tool in this case: The location of the tables is in fact not determined by the autotoolised build system, but it is determined at run time -- and moreover the arithmetic tables are first created by spkg-postinst, not by the makefile of meataxe.

comment:26 in reply to: ↑ 25 ; follow-up: Changed 22 months ago by dimpase

Replying to SimonKing:

Thank you, Dima!

I wonder: Should I add pkg-config to SharedMeatAxe as well? After all, meataxe uses arithmetic tables that are of course stored in some default folder, and currently its location is presented by an environment variable. This variable is needed by the meataxe executables (that are also part of the package), whereas libmtx uses a C variable for the same purpose.

It certainly simplifies things, to be able to use pkg-config to check the library. E.g. imagine meataxe library available as a Debian package, and then naturally one would want to use it rather than to build Sage's one...

As to location of the tables/databases, naturally, there is no standard. Although I imagine there are defaults set in libmtx and in meataxe executables.

I took this design from the original MeatAxe from which I forked off SharedMeatAxe, but I've never been happy with it.

On the other hand, I'm not sure if pkg-config would be the right tool in this case: The location of the tables is in fact not determined by the autotoolised build system, but it is determined at run time -- and moreover the arithmetic tables are first created by spkg-postinst, not by the makefile of meataxe.

How is this determination happening at runtime? Checking an environment variable? Or there is a default value?

comment:27 in reply to: ↑ 26 Changed 22 months ago by SimonKing

Replying to dimpase:

It certainly simplifies things, to be able to use pkg-config to check the library. E.g. imagine meataxe library available as a Debian package, and then naturally one would want to use it rather than to build Sage's one...

However, the location of the arithmetic tables does not depend on who built it. If the tables can not be found in the location specified by the environment variable MTXLIB resp. the C variable MtxLibDir, then it is attempted to read from pwd. And if this fails, it is attempted create new tables.

As to location of the tables/databases, naturally, there is no standard. Although I imagine there are defaults set in libmtx and in meataxe executables.

The default is the current directory (so, better not use the default!).

How is this determination happening at runtime? Checking an environment variable? Or there is a default value?

Actually I don't recall if there is a default value of the variable. You as a user of meataxe executables on the command line are supposed to assign a value to the environment variable, and you as a programmer of code linked against libmtx are supposed to assign a value to the C variable. Sage does both automatically, if you import from sage.libs.meataxe.

So, the user (Sage, in this case) has full control. If one fails to assign a value, then the current directory is used, or an error results (I don't recall).

comment:28 in reply to: ↑ 24 Changed 22 months ago by mkoeppe

Replying to dimpase:

By the way, you can have a custom variable in .pc file - in your case, say, the path to the database - and request it using pkg-config --variable=... ... calls (and from Python pkgconfig in a similar way)

This is a great point that I had forgotten about -- so pkg-config is a great solution for advertising the location of the data files too.

comment:29 follow-up: Changed 22 months ago by mkoeppe

I would suggest deferring the discussion about meataxe to a separate ticket

comment:30 in reply to: ↑ 29 Changed 22 months ago by SimonKing

Replying to mkoeppe:

I would suggest deferring the discussion about meataxe to a separate ticket

I wanted to know whether a new ticket makes sense at all, before opening it.

comment:31 Changed 22 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:32 Changed 21 months ago by mkoeppe

  • Keywords sd111 added

Hoping we can make progress on this ticket this week - https://wiki.sagemath.org/days111

comment:33 Changed 18 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:34 Changed 13 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:35 Changed 12 months ago by mkoeppe

  • Dependencies #28711 deleted

comment:36 follow-up: Changed 11 months ago by dimpase

while working on #31404, and trying p_group_cohomology in the new setup, I saw that Singular has renamed poly.lib to polylib.lib. This causes errors in the doctests, with Singular 4.2. Maybe a special ticket for such things? Also the format of dickson.lib is old:

> LIB "dickson.lib";
// ** loaded /mnt/opt/Sage/sage-dev/local/share/singular/LIB/dickson.lib 
// ** library dickson.lib has old format. This format is still accepted,
// ** but for functionality you may wish to change to the new
// ** format. Please refer to the manual for further information.
// ** loaded /usr/bin/../share/singular/LIB/polylib.lib (4.2.0.0,Dec_2020)
// ** redefining ringweights (LIB "dickson.lib";)
// ** redefining ringweights (LIB "dickson.lib";)
// ** loaded /usr/bin/../share/singular/LIB/ring.lib (4.1.2.0,Feb_2019)
// ** loaded /usr/bin/../share/singular/LIB/primdec.lib (4.2.0.0,Mar_2021)
// ** loaded /usr/bin/../share/singular/LIB/absfact.lib (4.1.2.0,Feb_2019)
// ** loaded /usr/bin/../share/singular/LIB/triang.lib (4.1.2.0,Feb_2019)
// ** loaded /usr/bin/../share/singular/LIB/matrix.lib (4.1.2.0,Feb_2019)
// ** loaded /usr/bin/../share/singular/LIB/nctools.lib (4.1.2.0,Feb_2019)
// ** loaded /usr/bin/../share/singular/LIB/random.lib (4.1.2.0,Feb_2019)
// ** loaded /usr/bin/../share/singular/LIB/elim.lib (4.1.2.0,Feb_2019)
// ** loaded /usr/bin/../share/singular/LIB/inout.lib (4.1.2.0,Feb_2019)
// ** loaded /usr/bin/../share/singular/LIB/general.lib (4.1.2.0,Feb_2019)
> 

comment:37 in reply to: ↑ 36 Changed 11 months ago by mkoeppe

Replying to dimpase:

while working on #31404, and trying p_group_cohomology in the new setup, I saw that Singular has renamed poly.lib to polylib.lib.

That's https://github.com/sagemath/p_group_cohomology/issues/4

comment:38 Changed 10 months ago by culler

For what it is worth, I am attaching a patch for p_group_cohomology/spkg-install.in and a patch (include_dir.patch) to be installed in p_group_cohomology/patches/.

I was able to build the spkg with these changes.

Changed 10 months ago by culler

patch for p_group_cohomology/spkg-install.in

Changed 10 months ago by culler

patch to install in p_group_cohomology/patches

comment:39 Changed 8 months ago by mkoeppe

  • Milestone changed from sage-9.5 to sage-9.6

comment:40 Changed 5 months ago by dimpase

p_group_cohomology build fails on Linux 9.6.beta3, with or without the @culler's patches. I opened #33474 to fix this.

comment:41 Changed 5 months ago by mkoeppe

  • Branch set to u/mkoeppe/package_modular_resolution__split_out_from_p_group_cohomology

comment:42 Changed 5 months ago by mkoeppe

  • Commit set to f94115246a19dcdeef8c82d9fa4ebd93558a0f39

Here's a beginning


New commits:

f941152build/pkgs/modular_resolution: New, split from p_group_cohomology

comment:43 Changed 5 months ago by mkoeppe

  • Authors set to Matthias Koeppe, ...

comment:44 Changed 5 months ago by git

  • Commit changed from f94115246a19dcdeef8c82d9fa4ebd93558a0f39 to 364f839ab06d42dd4ffd0ffe8b52ce1b15e102e3

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

364f839build/pkgs/{p_group_cohomology,modular_resolution}: First attempt at dependencies

comment:45 Changed 5 months ago by git

  • Commit changed from 364f839ab06d42dd4ffd0ffe8b52ce1b15e102e3 to fce05cacfa716abe6836af79bbea2d572440362b

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

fce05cabuild/pkgs/p_group_cohomology/spkg-install.in: Remove installation of modular_resolution

comment:46 Changed 5 months ago by mkoeppe

untested, feel free to continue. getting late here.

comment:47 Changed 5 months ago by dimpase

  • Authors changed from Matthias Koeppe, ... to Matthias Koeppe, Dima Pasechnik
  • Branch changed from u/mkoeppe/package_modular_resolution__split_out_from_p_group_cohomology to u/dimpase/package_modular_resolution__split_out_from_p_group_cohomology
  • Commit changed from fce05cacfa716abe6836af79bbea2d572440362b to 0b92e848c372559142506e0e88fc0ddeeb52d60a

OK, this builds - but the GAP helper files aren't ending in the correct place. Thus this fix. Still need to sort of dickson.lib, as the system-wide Singular doesn't find it.


New commits:

0b92e84move installing GAP and Singular files to spkg-postinstall

comment:48 Changed 5 months ago by dimpase

OK, the following tells Singular where to look for:

dima@hilbert /mnt/opt/Sage/sage-dev $ export SINGULARPATH=`pwd`/local/share/singular/LIB
dima@hilbert /mnt/opt/Sage/sage-dev $ ./sage --singular
                     SINGULAR                                 /  Development
 A Computer Algebra System for Polynomial Computations       /   version 4.2.1
                                                           0<
 by: W. Decker, G.-M. Greuel, G. Pfister, H. Schoenemann     \   May 2021
FB Mathematik der Universitaet, D-67653 Kaiserslautern        \
> LIB "dickson.lib";
// ** loaded /mnt/opt/Sage/sage-dev/local/share/singular/LIB/dickson.lib 
// ** library dickson.lib has old format. This format is still accepted,
// ** but for functionality you may wish to change to the new
// ** format. Please refer to the manual for further information.
   ? cannot open `poly.lib`
// ** loaded /usr/bin/../share/singular/LIB/general.lib (4.1.2.0,Feb_2019)
...

And we're running into an old name for polylib.lib.

comment:49 Changed 5 months ago by git

  • Commit changed from 0b92e848c372559142506e0e88fc0ddeeb52d60a to 45e67799f907c8e06ba24fd99d8af6cc00226315

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

45e6779patch to change poly.lib -> polylib.lib

comment:50 follow-up: Changed 5 months ago by dimpase

OK, so now we are in a reasonable shape; one doctest fails due to deprecation of set_default_prec(), used in upstream/p_group_cohomology-3.3.2/pGroupCohomology-3.3.2/pGroupCohomology/barcode.py, the rest work.

And we need to sort out how to deal with SINGULARPATH - cf. comment:48.

comment:51 Changed 5 months ago by dimpase

  • Priority changed from major to blocker
  • Status changed from new to needs_info

comment:52 in reply to: ↑ 50 Changed 5 months ago by mkoeppe

Replying to dimpase:

And we need to sort out how to deal with SINGULARPATH - cf. comment:48.

Don't try to adjust SINGULARPATH, just load the library from the absolute pathname

comment:53 Changed 5 months ago by mkoeppe

Likewise for the gap helper, don't try to install it in the GAP library, just load it from the absolute pathname

comment:54 follow-up: Changed 5 months ago by jhpalmieri

I installed this on OS X, although the first time failed in building modular_resolution:

./modular_resolution.h:29:10: fatal error: 'meataxe.h' file not found
#include "meataxe.h"
         ^~~~~~~~~~~
1 error generated.

Maybe add meataxe to the dependencies file for modular_resolution?

After building everything, the test suite failed almost immediately with

Running the test suite for p_group_cohomology-3.3.2.p0...
make[3]: *** No rule to make target `check'.  Stop.
********************************************************************************
Error testing modular_resolution-1.1
********************************************************************************

real	0m0.134s
user	0m0.016s
sys	0m0.025s
************************************************************************
Error testing package p_group_cohomology-3.3.2.p0
************************************************************************

The package has an extensive test suite, so if we can get it to pass tests, that should be a strong indication that everything is working.

comment:55 Changed 5 months ago by mkoeppe

  • Branch changed from u/dimpase/package_modular_resolution__split_out_from_p_group_cohomology to u/mkoeppe/package_modular_resolution__split_out_from_p_group_cohomology

comment:56 Changed 5 months ago by git

  • Commit changed from 45e67799f907c8e06ba24fd99d8af6cc00226315 to d82f5332b0667ae84e57ae2e2d3b985e23450aad

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

0b92e84move installing GAP and Singular files to spkg-postinstall
45e6779patch to change poly.lib -> polylib.lib
d82f533build/pkgs/modular_resolution/dependencies: New

comment:57 in reply to: ↑ 54 Changed 5 months ago by mkoeppe

Replying to jhpalmieri:

Maybe add meataxe to the dependencies file for modular_resolution?

Here it is. I had forgotten to add the file

comment:58 Changed 5 months ago by jhpalmieri

The documentation doesn't build:

Running Sphinx v4.4.0

Extension error:
Could not import extension inventory_builder (exception: No module named 'inventory_builder')
make[3]: *** [html] Error 2
Last edited 5 months ago by jhpalmieri (previous) (diff)

comment:59 Changed 5 months ago by jhpalmieri

Patching conf.py fixed it:

  • pGroupCohomology-3.3.2/doc/source/conf.py

    diff --git a/pGroupCohomology-3.3.2/doc/source/conf.py b/pGroupCohomology-3.3.2/doc/source/conf.py
    index e24ceb6..ee16257 100644
    a b sys.path.append(os.path.join(SAGE_SRC, "sage_setup", "docbuild", "ext")) 
    3434# Add any Sphinx extension module names here, as strings. They can be
    3535# extensions coming with Sphinx (named 'sphinx.ext.*') or your custom
    3636# ones.
    37 extensions = ['inventory_builder',
    38               'sage_autodoc',
     37extensions = ['sage_docbuild.ext.inventory_builder',
     38              'sage_docbuild.ext.sage_autodoc',
    3939              'sphinx.ext.intersphinx',
    4040              'sphinx.ext.todo']

but I wonder if the p_group_cohomology conf.py should just point to Sage's conf.py and use that.

comment:60 Changed 5 months ago by tscrim

  • Cc tscrim added

comment:61 Changed 5 months ago by dimpase

  • Branch changed from u/mkoeppe/package_modular_resolution__split_out_from_p_group_cohomology to u/dimpase/package_modular_resolution__split_out_from_p_group_cohomology
  • Commit changed from d82f5332b0667ae84e57ae2e2d3b985e23450aad to 981836e15e668d8c7e6400553571bd6e7f13b3d5
  • Status changed from needs_info to needs_work

testsuite for modular_resolution (created in the latest commit) passes.

With p_group_cohomology, somehow I can't uninstall it, for some reason.


New commits:

981836esplit spkg-check between corresponding packages

comment:62 Changed 5 months ago by dimpase

Doing

export SINGULARPATH=`pwd`/local/share/singular/LIB

and after some fighting ./configure and using ./sage -f instead of make, I was able to rebuild p_group_cohomology and ran its testsuite. 2 failures - one due to deprecated set_default_prec(), clear how to fix, and another

[p_group_cohomology-3.3.2.p0] File "pGroupCohomology-3.3.2/pGroupCohomology/cohomology.pyx", line 993, in pGroupCohomology.cohomology.is_filter_regular
[p_group_cohomology-3.3.2.p0] Failed example:
[p_group_cohomology-3.3.2.p0]     is_filter_regular(H.relation_ideal(), F[0])
[p_group_cohomology-3.3.2.p0] Expected:
[p_group_cohomology-3.3.2.p0]     False
[p_group_cohomology-3.3.2.p0] Got:
[p_group_cohomology-3.3.2.p0]     [0]

I am not sure about. Looks like a genuine change.

comment:63 Changed 5 months ago by git

  • Commit changed from 981836e15e668d8c7e6400553571bd6e7f13b3d5 to 2b0096c70b49d76198ff83be492a1258a633a01b

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

2b0096cdoctest patches

comment:64 Changed 5 months ago by dimpase

more doctest etc patches for the package. This reduces the noise, but there are still quite a lot of discrepancies, some of which look serious. Also,

sage -t --warn-long 62.5 --random-seed=230138939486715312795148381621394057005 src/sage/tests/modular_group_cohomology.py
**********************************************************************
File "src/sage/tests/modular_group_cohomology.py", line 55, in sage.tests.modular_group_cohomology
Failed example:
    ascii_art(H.bar_code('LowerCentralSeries')[2])# optional - p_group_cohomology
Expected:
        *
      *-*
      *-*
    *
Got:
      *-*
      *-*
      *-*
      *-*
    *
**********************************************************************
1 item had failures:
   1 of  18 in sage.tests.modular_group_cohomology
    [17 tests, 1 failure, 3.71 s]

John, do you have an idea about test failures?

comment:65 Changed 5 months ago by SimonKing

First of all: Thank you very much for working on it! I am currently buried in exam grading and unable to do more interesting stuff.

Both the failure at is_filter_regular(H.relation_ideal(), F[0]) and H.bar_code('LowerCentralSeries') look very serious to me. So, I should try and build stuff, and try to analyse the failures.

My question: It looks to me as if the branch introduces patches -- I guess these should eventually be included in the next version of the p_group_cohomology package, i.e., should be pushed to the github repository of the package. But how? Could you tell me a workflow?

Best regards, Simon

comment:66 Changed 5 months ago by dimpase

Hi Simon, patches are a bit messy, as they are against the package, which has a different directory structure. So a straight git apply <patch-file> doesn't work in the upstream directory, one needs to use patch -pX with different values of X for different files. Don't worry about this, I'll push a branch upstream with these changes applied.

After that, for the new release we'd also need to sort out packaging of GAP and Singular files, as Matthias says. But it's certainly most important to fix the maths issues!

It's already possible to build and test this branch, with workaround in comment:62

comment:67 Changed 5 months ago by dimpase

  • Report Upstream changed from N/A to Fixed upstream, in a later stable release.

comment:68 Changed 5 months ago by mkoeppe

  • Branch changed from u/dimpase/package_modular_resolution__split_out_from_p_group_cohomology to u/mkoeppe/package_modular_resolution__split_out_from_p_group_cohomology

comment:69 Changed 5 months ago by mkoeppe

  • Commit changed from 2b0096c70b49d76198ff83be492a1258a633a01b to bd144964a992530c3f0fda812708984e86a675d4

Do I understand correctly that the only place (other than install scripts) that knows about the installation location of the database is set_local_sources ( https://github.com/sagemath/p_group_cohomology/blob/master/src/pGroupCohomology/factory.py#L617)?


New commits:

bd14496build/pkgs/modular_resolution/spkg-install.in: Use sdh_make_install, remove redundant error handling

comment:70 Changed 5 months ago by git

  • Commit changed from bd144964a992530c3f0fda812708984e86a675d4 to 53198c8888822f35dcc4aaedd9fb7c8891966edc

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

53198c8build/pkgs/modular_resolution/SPKG.rst: Update for separate package

comment:71 Changed 5 months ago by jhpalmieri

  • Branch changed from u/mkoeppe/package_modular_resolution__split_out_from_p_group_cohomology to u/jhpalmieri/package_modular_resolution__split_out_from_p_group_cohomology

comment:72 Changed 5 months ago by jhpalmieri

  • Commit changed from 53198c8888822f35dcc4aaedd9fb7c8891966edc to 7b994c765c2a91664f562355cff7dc7931d57939

This should fix docbuilding.


New commits:

7b994c7trac 30787: fix docbuilding. Mainly use Sage's conf.py.

comment:73 Changed 5 months ago by mkoeppe

  • Authors changed from Matthias Koeppe, Dima Pasechnik to Matthias Koeppe, Dima Pasechnik, John Palmieri

comment:74 Changed 5 months ago by dimpase

  • Branch changed from u/jhpalmieri/package_modular_resolution__split_out_from_p_group_cohomology to u/dimpase/package_modular_resolution__split_out_from_p_group_cohomology
  • Commit changed from 7b994c765c2a91664f562355cff7dc7931d57939 to a48b1ffce764ef0fd4efd1026e9045aea929ccce

New commits:

a48b1ffa separate repo and a tarball for modular_resolution

comment:75 Changed 5 months ago by git

  • Commit changed from a48b1ffce764ef0fd4efd1026e9045aea929ccce to 8768aa4cde67075f6a01d4b45ccc69080c11934c

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

8768aa4upstream patches, new version

comment:76 follow-up: Changed 5 months ago by dimpase

OK, refactoring done, what remains are maths errors.

comment:77 in reply to: ↑ 76 Changed 5 months ago by SimonKing

Replying to dimpase:

OK, refactoring done, what remains are maths errors.

Thank you! So, what's the plan now? I suppose I have to pull from the upstream repository to which you posted the new version, and then try to install it on my laptop, so that I can see what is happening on the mathematical side.

Question: Has there been a recent Singular upgrade in Sage? If so: What version of Sage?

comment:78 Changed 5 months ago by SimonKing

I just tried to pull, but I got permission denied. It could be that I am on a new laptop now. So, how can I convince git that I have permission to pull and push? Both for the upstream repository and for the sage trac please.

comment:79 follow-up: Changed 5 months ago by dimpase

you probably don't need the upstream repo now. (for github (and for git trac) you might need to create a new ssh keypair, some old keys are now deemed too insecure)

Just build the branch of this ticket.

Last edited 5 months ago by dimpase (previous) (diff)

comment:80 Changed 5 months ago by dimpase

Singular now is 4.2.0 or 1, don't recall. (away from kbd now)

comment:81 Changed 5 months ago by dimpase

You can also run this branch in GitPod (the rightmost button in Status Badges above) You'll get a VM with this branch pulled (but without p_group_cohomology built, so you'd need to do make p_group_cohomology followed up by make build in the terminal window there). Takes 5-10 minutes to start.

You'd need to be authenticated to GitHub, I guess, to use it.

comment:82 Changed 5 months ago by dimpase

Oops, it seems that my latest change to the package created a circular import (only detected from a fresh build it seems)

See https://github.com/sagemath/p_group_cohomology/commit/3a7a043fe259b7ce667ad6b8f8d8ab0ea42a9fa4 One cannot import a package inside itself, I gather, but without this I can't call os.path.dirname(pGroupCohomology.__file__). Any way out here?

comment:83 Changed 5 months ago by dimpase

I guess, just use __file__ in these calls, and remove import statements.

comment:84 Changed 5 months ago by mkoeppe

yes

comment:85 Changed 5 months ago by mkoeppe

  • Description modified (diff)

comment:86 Changed 5 months ago by mkoeppe

  • Description modified (diff)

comment:87 Changed 5 months ago by git

  • Commit changed from 8768aa4cde67075f6a01d4b45ccc69080c11934c to 2ee298a718b5d3bf0c4bbdfe26ed4a0345b5895b

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

2ee298apackage update - to use __file__

comment:88 Changed 5 months ago by git

  • Commit changed from 2ee298a718b5d3bf0c4bbdfe26ed4a0345b5895b to 9dc4be937c6fb5305928b42a05845afbcecb8dbb

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

dba7b11move installing GAP and Singular files to spkg-postinstall
3d19711patch to change poly.lib -> polylib.lib
b4b8a3ebuild/pkgs/modular_resolution/dependencies: New
b1c6bc6split spkg-check between corresponding packages
d135185doctest patches
17dc21cbuild/pkgs/modular_resolution/spkg-install.in: Use sdh_make_install, remove redundant error handling
272c144trac 30787: fix docbuilding. Mainly use Sage's conf.py.
1d295f7a separate repo and a tarball for modular_resolution
03190b6upstream patches, new version
9dc4be9package update - to use __file__

comment:89 in reply to: ↑ 79 ; follow-up: Changed 5 months ago by SimonKing

Replying to dimpase:

you probably don't need the upstream repo now.

At some point (namely when fixing the maths problem) I need to be able to push.

(for github (and for git trac) you might need to create a new ssh keypair, some old keys are now deemed too insecure)

How? I'm logged in to my github account and visited my profile, but I see no hint how to upload my public ssh key. Also it is a long time since I last created an ssh key. How do I do this? On sage-devel, I saw that some special type of key is now needed.

Just build the branch of this ticket.

This wouldn't allow me to work an the maths problem.


Last 10 new commits:

dba7b11move installing GAP and Singular files to spkg-postinstall
3d19711patch to change poly.lib -> polylib.lib
b4b8a3ebuild/pkgs/modular_resolution/dependencies: New
b1c6bc6split spkg-check between corresponding packages
d135185doctest patches
17dc21cbuild/pkgs/modular_resolution/spkg-install.in: Use sdh_make_install, remove redundant error handling
272c144trac 30787: fix docbuilding. Mainly use Sage's conf.py.
1d295f7a separate repo and a tarball for modular_resolution
03190b6upstream patches, new version
9dc4be9package update - to use __file__

comment:90 Changed 5 months ago by dimpase

still an error to fix for the GAP package path. Should be ready soon.

comment:91 in reply to: ↑ 89 Changed 5 months ago by dimpase

Replying to SimonKing:

Replying to dimpase:

you probably don't need the upstream repo now.

At some point (namely when fixing the maths problem) I need to be able to push.

(for github (and for git trac) you might need to create a new ssh keypair, some old keys are now deemed too insecure)

How? I'm logged in to my github account and visited my profile, but I see no hint how to upload my public ssh key.

It's in Settings, not in Profile.

Also it is a long time since I last created an ssh key. How do I do this? On sage-devel, I saw that some special type of key is now needed.

Nothing special, just secure by modern standards. Either a long RSA key (say, 4096 bits), or an ed25519 key. The command to run for the latter is ssh-keygen -t ed25519

Just build the branch of this ticket.

This wouldn't allow me to work an the maths problem.

one can make patches against the package tarball, instead of re-creating the tarball at the source repo. Anyway, you should be able to get your keys right.

comment:92 Changed 5 months ago by git

  • Commit changed from 9dc4be937c6fb5305928b42a05845afbcecb8dbb to 491631219f4372cc093298ee94826296c532b20d

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

4916312package update to fix path to GAP package

comment:93 Changed 5 months ago by tscrim

Is this currently expected to be able to be installed? When I tried make modular_resolution, I just get that it couldn't find the make target, similar for sage -i modular_resolution. It is being picked up by sage --optional (and I did also run ./configure and make build before trying this too). Am I just doing something stupid (like forgetting a step)?

comment:94 Changed 5 months ago by dimpase

Perhaps it needs a run of ./bootstrap && ./configure --enable-download-from-upstream-url? It works for me now from a clean build - and it works even in GitPod.

comment:95 Changed 5 months ago by dimpase

modular_resolution by itself is not interesting, try p_group_cohomology.

comment:96 Changed 5 months ago by dimpase

Apparently, GitPod allows creating snapshots - so that's what I just have got there on this branch:

https://gitpod.io#snapshot/a3cc9419-a976-446c-8247-1f2d84b4cabc

(no idea for how long it persists)

comment:97 Changed 5 months ago by tscrim

I was not doing the ./bootstrap, which I didn't think was necessary since I was adding this to my already built Sage. That fixed it.

comment:98 follow-up: Changed 5 months ago by jhpalmieri

During compilation, I see a bunch of warnings ("unused variable" or "unused function"). Do we care about these?

comment:99 in reply to: ↑ 98 Changed 5 months ago by SimonKing

Replying to jhpalmieri:

During compilation, I see a bunch of warnings ("unused variable" or "unused function"). Do we care about these?

I do. I'm afraid that it is a long time since last working on the package, but getting rid of warnings was one of my concerns.

comment:100 Changed 5 months ago by mkoeppe

Should we just merge this ticket, as going from "cannot be installed" to "can be installed" is already an improvement?

comment:101 Changed 5 months ago by dimpase

Yes, it would be good to have it in 9.6, fixed, preferably (or else failing tests should be marked as bugs)

comment:102 Changed 5 months ago by mkoeppe

  • Priority changed from blocker to critical

comment:103 Changed 4 months ago by mkoeppe

  • Milestone changed from sage-9.6 to sage-9.7

comment:104 Changed 11 days ago by mkoeppe

  • Milestone changed from sage-9.7 to sage-9.8

comment:105 Changed 8 days ago by jhpalmieri

See this sage-devel message for a fix to build p_group_cohomology, along with #34221.

comment:106 Changed 8 days ago by mkoeppe

  • Branch changed from u/dimpase/package_modular_resolution__split_out_from_p_group_cohomology to u/mkoeppe/package_modular_resolution__split_out_from_p_group_cohomology

comment:107 Changed 8 days ago by git

  • Commit changed from 491631219f4372cc093298ee94826296c532b20d to 85c0baec5c4a559217ceceb988501b0f331e40ff

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

7e72b2abuild/pkgs/cython/patches/4918.patch: New
176531abuild/pkgs/cython/package-version.txt: Add patchlevel
48a9ba2Merge #34237
85c0baeMerge #34221

comment:108 Changed 8 days ago by mkoeppe

  • Dependencies set to #34221

comment:109 Changed 8 days ago by mkoeppe

sage -t --random-seed=154551441816182074078932648792662937529 src/sage/tests/modular_group_cohomology.py
**********************************************************************
File "src/sage/tests/modular_group_cohomology.py", line 10, in sage.tests.modular_group_cohomology
Failed example:
    from pGroupCohomology import CohomologyRing   # optional - p_group_cohomology
Exception raised:
    Traceback (most recent call last):
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 695, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 1093, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.tests.modular_group_cohomology[0]>", line 1, in <module>
        from pGroupCohomology import CohomologyRing   # optional - p_group_cohomology
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/local/var/lib/sage/venv-python3.8/lib/python3.8/site-packages/pGroupCohomology/__init__.py", line 2466, in <module>
        from pGroupCohomology.factory import CohomologyRing
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/local/var/lib/sage/venv-python3.8/lib/python3.8/site-packages/pGroupCohomology/factory.py", line 45, in <module>
        from pGroupCohomology.cohomology import COHO
      File "pGroupCohomology/cohomology.pyx", line 71, in init pGroupCohomology.cohomology
        from sage.docs.instancedoc import instancedoc
    ImportError: cannot import name instancedoc

comment:110 Changed 8 days ago by mkoeppe

  • Cc klee added

Broken by #33763?

comment:111 Changed 8 days ago by tscrim

Indeed, we need to change

-from sage.docs.instancedoc import instancedoc
+from sage.misc.instancedoc import instancedoc

upstream.

comment:112 Changed 8 days ago by mkoeppe

Looks like the deprecation done in #33763 somehow doesn't work

comment:113 follow-up: Changed 8 days ago by tscrim

Maybe because it is a Cython file? It could be converted to a .py file, but then I don't get a deprecation message. Probably the best thing would be to display a deprecation message when sage.docs.instancedoc (maybe also changed to a .py file) is imported and the redirect is a normal import.

Well, this still will need to be fixed upstream anyways.

comment:114 Changed 8 days ago by mkoeppe

I've opened #34324 (Fix deprecated import of instancedoc)

comment:115 in reply to: ↑ 113 Changed 8 days ago by mkoeppe

Replying to tscrim:

Well, this still will need to be fixed upstream anyways.

That's now https://github.com/sagemath/p_group_cohomology/pull/7

comment:116 Changed 8 days ago by git

  • Commit changed from 85c0baec5c4a559217ceceb988501b0f331e40ff to 8803c93920bc207acaac1cca98841534652cdbd6

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

8803c93build/pkgs/p_group_cohomology/patches: Add https://github.com/sagemath/p_group_cohomology/pull/7

comment:117 Changed 8 days ago by git

  • Commit changed from 8803c93920bc207acaac1cca98841534652cdbd6 to 27d6f7fdf0329e02eaa9906707c0ae4feaed6ee3

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

27d6f7fbuild/pkgs/p_group_cohomology/patches: Add https://github.com/sagemath/p_group_cohomology/pull/7

comment:118 Changed 8 days ago by git

  • Commit changed from 27d6f7fdf0329e02eaa9906707c0ae4feaed6ee3 to 9579fb4325bc2779dc7d2b16c5d939260d01b7c3

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

9579fb4build/pkgs/p_group_cohomology/patches: Add https://github.com/sagemath/p_group_cohomology/pull/7

comment:119 follow-up: Changed 8 days ago by mkoeppe

This fixes it.

We are now back to the same status as comment:100, 5 months ago:

  • it builds
  • package testsuite (make SAGE_CHECK=warn p_group_cohomology) has failures
  • 1 failure in ./sage -t src/sage/tests/modular_group_cohomology.py

I'd suggest that we merge it as is.

comment:120 Changed 8 days ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:121 Changed 8 days ago by jhpalmieri

I think that it should have #34291 as a dependency and set this back to optional.

comment:122 Changed 8 days ago by mkoeppe

  • Dependencies changed from #34221 to #34221, #34291

comment:123 Changed 8 days ago by git

  • Commit changed from 9579fb4325bc2779dc7d2b16c5d939260d01b7c3 to 0de2c986e8b8252e996b6905348ac64ae268fe20

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

43bda83trac 34291: downgrade some broken optional packages to experimental
de94e31Merge #34291
0de2c98build/pkgs/p_group_cohomology/type: Back to optional

comment:124 follow-up: Changed 7 days ago by tscrim

What would be your recommended way to test this package? Of course, I can do pull requests to fix things like

File "pGroupCohomology-3.3.3/pGroupCohomology/factory.py", line 909, in pGroupCohomology.factory.CohomologyRingFactory.create_group_key
Failed example:
    type(X._key[0][0])
Expected:
    <type 'sage.rings.integer.Integer'>
Got:
    <class 'sage.rings.integer.Integer'>

but if I wanted to start looking at the more serious bugs, what would be the most efficient way to test it? I imagine doing patches and rebuilding the spkg is not ideal.

@SimonKing? If you have any comments, both on the mathematical side or the development side, they would be appreciated!

comment:125 Changed 7 days ago by SimonKing

I very much appreciate that you work on these things. Because of teaching, I didn't have much time recently. Actually, even though my SageMath installation is still working on my laptop, git claims that there is no git repository in the source tree. Hence, I couldn't even upgrade my SageMath installation. Today I finished a new build from scratch.

Next, I probably need to be reminded how to access/use the github repository of the cohomology package...

comment:126 in reply to: ↑ 124 Changed 7 days ago by mkoeppe

Replying to tscrim:

What would be your recommended way to test this package? Of course, I can do pull requests to fix things like

File "pGroupCohomology-3.3.3/pGroupCohomology/factory.py", line 909, in pGroupCohomology.factory.CohomologyRingFactory.create_group_key
Failed example:
    type(X._key[0][0])
Expected:
    <type 'sage.rings.integer.Integer'>
Got:
    <class 'sage.rings.integer.Integer'>

but if I wanted to start looking at the more serious bugs, what would be the most efficient way to test it? I imagine doing patches and rebuilding the spkg is not ideal.

You can just go into a git checkout of https://github.com/sagemath/p_group_cohomology, and do sage -pip install src/

comment:127 follow-ups: Changed 7 days ago by tscrim

comment:125: No problem; I think we can all relate. ;) It sounds like you might have accidentally deleted the .git folder. You can run

git clone git@github.com:sagemath/p_group_cohomology.git

(assuming you will access github via ssh; I got this from the (on my screen it is green) "code" button) from whatever folder you want to get the source code. You might even have direct push/pull access.

comment:126: Thank you. I am guessing I should run that command from the folder I put the pGpCoHo package?

comment:128 in reply to: ↑ 127 ; follow-up: Changed 7 days ago by mkoeppe

Replying to tscrim:

comment:126: Thank you. I am guessing I should run that command from the folder I put the pGpCoHo package?

Yes, that's what I meant by "go into"

comment:129 in reply to: ↑ 128 Changed 7 days ago by tscrim

Replying to mkoeppe:

Replying to tscrim:

comment:126: Thank you. I am guessing I should run that command from the folder I put the pGpCoHo package?

Yes, that's what I meant by "go into"

Ah, sorry I missed that. Thank you.

Changed 7 days ago by tscrim

Current failures of 3.3.3p1 version

comment:130 in reply to: ↑ 127 ; follow-up: Changed 7 days ago by SimonKing

Replying to tscrim:

comment:125: No problem; I think we can all relate. ;) It sounds like you might have accidentally deleted the .git folder.

That would come as a surprise. Anyway, I used several worktrees, and I see a .git file (not a folder) whose content is gitdir: /home/king/Sage/git/sage/.git/worktrees/py3

Anyway, now I have a new Sage installation, I see not an urgent need to fix the old one, although I won't delete the old installations.

You can run

git clone git@github.com:sagemath/p_group_cohomology.git

(assuming you will access github via ssh;

Meanwhile I have. Strange enough: I used to work with Sage on this laptop, but I needed to upload the public ssh key that I have on this laptop.

comment:131 in reply to: ↑ 130 Changed 7 days ago by tscrim

Replying to SimonKing:

Replying to tscrim:

comment:125: No problem; I think we can all relate. ;) It sounds like you might have accidentally deleted the .git folder.

That would come as a surprise. Anyway, I used several worktrees, and I see a .git file (not a folder) whose content is gitdir: /home/king/Sage/git/sage/.git/worktrees/py3

That is very strange.

Anyway, now I have a new Sage installation, I see not an urgent need to fix the old one, although I won't delete the old installations.

Indeed, it is not worth the trouble.

Meanwhile I have. Strange enough: I used to work with Sage on this laptop, but I needed to upload the public ssh key that I have on this laptop.

That is because Github is not trac. They are different servers and both require your (public) ssh key.

comment:132 Changed 7 days ago by mkoeppe

  • Description modified (diff)
  • Reviewers set to ..., Matthias Koeppe

comment:133 in reply to: ↑ 119 Changed 7 days ago by mkoeppe

Replying to mkoeppe:

We are now back to the same status as comment:100, 5 months ago:

  • it builds
  • package testsuite (make SAGE_CHECK=warn p_group_cohomology) has failures
  • 1 failure in ./sage -t src/sage/tests/modular_group_cohomology.py

I'd suggest that we merge it as is.

I've opened #34333 for fixing the tests.

comment:134 Changed 7 days ago by jhpalmieri

  • Reviewers changed from ..., Matthias Koeppe to John Palmieri, Matthias Koeppe

I agree that we should merge it as is, since it now builds. I won't set to "positive review" yet in case anyone disagrees, but I'll change it in a few days if no one says anything (or does it first).

comment:135 Changed 7 days ago by dimpase

  • Reviewers changed from John Palmieri, Matthias Koeppe to John Palmieri, Matthias Koeppe, Dima Pasechnik
  • Status changed from needs_review to positive_review

it's good to go.

comment:136 Changed 7 days ago by mkoeppe

  • Milestone changed from sage-9.8 to sage-9.7

comment:137 Changed 7 days ago by mkoeppe

Thanks all!

comment:138 Changed 6 days ago by tscrim

  • Status changed from positive_review to needs_work

+1 on this as the errors are likely a little more involved to fix and it would be good to keep this optional. However, one thing before we include this is marking the doctest failure within Sage as a # known bug.

comment:139 Changed 6 days ago by mkoeppe

This was also demanded in comment:101, but who's going to work on it?

comment:140 Changed 6 days ago by mkoeppe

  • Status changed from needs_work to needs_review

comment:141 Changed 6 days ago by tscrim

It doesn't need to be fixed here, it is just having the doctest marked so we don't get the failure reported when running tests with the spkg installed.

comment:142 Changed 6 days ago by tscrim

I mean specifically the one in the Sage library noted in comment:64.

comment:143 Changed 6 days ago by mkoeppe

Outside the scope of the ticket to add such annotations.

comment:144 Changed 6 days ago by tscrim

  • Branch changed from u/mkoeppe/package_modular_resolution__split_out_from_p_group_cohomology to u/tscrim/modular_resolution_split-30787
  • Commit changed from 0de2c986e8b8252e996b6905348ac64ae268fe20 to 3641be4f8312f00193f810ed0c076783bb9ffc6c
  • Reviewers changed from John Palmieri, Matthias Koeppe, Dima Pasechnik to John Palmieri, Matthias Koeppe, Dima Pasechnik, Travis Scrimshaw

I disagree. I have pushed it.


New commits:

3641be4Marking failing test as "# known bug".

comment:145 Changed 6 days ago by mkoeppe

Do you know that it's a bug?

comment:146 follow-up: Changed 6 days ago by tscrim

No as I can't (quickly) find this in the literature (as I don't know this area very well). However, since it is in a tests file, I am operating under the assumption it has been verified through some other means. Furthermore, none of the other results have changed.

Simon, perhaps you can comment briefly on this?

comment:147 Changed 6 days ago by mkoeppe

Then please clarify the annotation from # known bug to # known bug - at least one of the results is wrong

comment:148 Changed 6 days ago by git

  • Commit changed from 3641be4f8312f00193f810ed0c076783bb9ffc6c to c7108e9a8f0ccf4fbc19f6cbfa799b5d6f347439

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

c7108e9Updating "# known bug" description.

comment:149 Changed 6 days ago by tscrim

Done.

comment:150 Changed 6 days ago by mkoeppe

This is fine with me.

comment:151 Changed 6 days ago by tscrim

  • Status changed from needs_review to positive_review

I am setting it back to a positive review then.

Simon (or John or someone else if you know), if you could comment on the correctness of the test output, that would be appreciated.

comment:152 in reply to: ↑ 146 ; follow-up: Changed 6 days ago by SimonKing

Replying to tscrim:

No as I can't (quickly) find this in the literature (as I don't know this area very well). However, since it is in a tests file, I am operating under the assumption it has been verified through some other means. Furthermore, none of the other results have changed.

Simon, perhaps you can comment briefly on this?

In the test, an invariant of the group is calculated. So, if there are two different results than either the result computed by the old package version or the result computed by the new package version is wrong (or both). Therefore certainly one of the two versions has a bug.

I could ask Graham Ellis (my co-author in a publication on "persistant group cohomology") if he can compute this example: I know that he has software developed independently of p_group_cohomology.

comment:153 in reply to: ↑ 152 Changed 6 days ago by tscrim

Replying to SimonKing:

I could ask Graham Ellis (my co-author in a publication on "persistant group cohomology") if he can compute this example: I know that he has software developed independently of p_group_cohomology.

Thank you for your thoughts. Could you please ask him? Independent corroboration would be great.

Note: See TracTickets for help on using tickets.