Semigroups icon indicating copy to clipboard operation
Semigroups copied to clipboard

Renaming PreImages... functions as PreImages...NC : GAP PR#5073

Open cdwensley opened this issue 2 years ago • 7 comments

PreImagesRepresetnative, PreImages, PreImagesSet and PreImagesElm can all return incorrect results when the element(s) supplied are not in the range of the map. This situation has been discussed in GAP issue #4809. To rectify the situation the plan is to have NC versions of these four operations and to add tests to the non-NC versions. The procedure to be adopted is as follows. (1) Rename the four operations by adding 'NC' to their names, and then declare the original operations as synonyms of these. PR #5073 addresses this. (2) Ask package authors/maintainers to convert all their calls to these functions to the NC versions (unless there is good reason not to do so). A clause added to init.g ensures the package works in older versions of GAP. (3) Once all the packages have been updated, add tests to the non-NC versions of the operations.

No progress has been made on this since February, but now seems a good time to continue with stage (2).
No PR is offered to Semigroups at the moment because it is not clear what to do. In attributes/homomorph.gi there are two methods implemented, and both of these apply tests, so should clearly not be converted to NC. On the other hand, in attributes/isorms.gi there are two new methods implemented which do not do tests, so perhaps should be converted to NC. Apart from these methods, there are just a few calls to PreImagesRepresentative. How would the package authors like to proceed?

cdwensley avatar Sep 12 '23 09:09 cdwensley

Thanks @cdwensley I’m happy to fix this thanks for the report!

james-d-mitchell avatar Sep 12 '23 18:09 james-d-mitchell

That's great. Tried three PRs today for various packages but suspect I'll hear nothing from the other two! This thing has dragged on far too long, which is entirely my fault, but a good push now might result in some progress. Maybe you can help with my query on Slack?

cdwensley avatar Sep 12 '23 19:09 cdwensley

This is clearly nowhere near the top of your TODO list, which is very understandable. So I will make a start on a PR - tell me if you want me to stop!

cdwensley avatar Oct 26 '23 19:10 cdwensley

But that's only if I can manage to compile Semigroups! I can get ./configure to work, but not make: christopherwensley@Christophers-iMac semigroups % ./configure --with-gaproot=/Users/christopherwensley/gap/gap-4.12.2 ... lots of output ... christopherwensley@Christophers-iMac semigroups % make make: *** No rule to make target Makefile.am', needed by Makefile.in'. Stop.

This is on an iMac less than two years old, so GCC and clang (whatever they are) should be OK. And I am working with a version of Semigroups cloned from GitHub.

cdwensley avatar Oct 27 '23 08:10 cdwensley

I'll welcome a pr with these changes! You need to ./autogen.sh in the semi groups directory before running configure.

james-d-mitchell avatar Nov 07 '23 08:11 james-d-mitchell

Yes, that was done first. The situation today is that 5.2.0 compiles just fine on my system, but not the GitHub version. So I am wondering if there is something missing from this website.
Anyway, I can make a start on a PR using the 5.2.0 version as a check. Here is today's log: christopherwensley@Christophers-iMac semigroups % ./autogen.sh ++ dirname ./autogen.sh

  • autoreconf -vif . autoreconf: export WARNINGS= autoreconf: Entering directory '.' autoreconf: configure.ac: not using Gettext autoreconf: running: aclocal --force autoreconf: configure.ac: tracing autoreconf: configure.ac: adding subdirectory libsemigroups to autoreconf autoreconf: Entering directory 'libsemigroups' autoreconf: configure.ac: not using Gettext autoreconf: running: aclocal --force -I m4 autoreconf: configure.ac: tracing autoreconf: running: glibtoolize --copy --force glibtoolize: putting auxiliary files in AC_CONFIG_AUX_DIR, 'config'. glibtoolize: copying file 'config/ltmain.sh' glibtoolize: putting macros in AC_CONFIG_MACRO_DIRS, 'm4'. glibtoolize: copying file 'm4/libtool.m4' glibtoolize: copying file 'm4/ltoptions.m4' glibtoolize: copying file 'm4/ltsugar.m4' glibtoolize: copying file 'm4/ltversion.m4' glibtoolize: copying file 'm4/lt~obsolete.m4' autoreconf: configure.ac: not using Intltool autoreconf: configure.ac: not using Gtkdoc autoreconf: running: aclocal --force -I m4 autoreconf: running: /opt/homebrew/Cellar/autoconf/2.71/bin/autoconf --force autoreconf: running: /opt/homebrew/Cellar/autoconf/2.71/bin/autoheader --force autoreconf: running: automake --add-missing --copy --force-missing configure.ac:20: installing 'config/compile' configure.ac:18: installing 'config/missing' Makefile.am: installing 'config/depcomp' autoreconf: Leaving directory 'libsemigroups' autoreconf: configure.ac: not using Libtool autoreconf: configure.ac: not using Intltool autoreconf: configure.ac: not using Gtkdoc autoreconf: running: /opt/homebrew/Cellar/autoconf/2.71/bin/autoconf --force configure.ac:46: error: possibly undefined macro: AC_SUBST If this token and others are legitimate, please use m4_pattern_allow. See the Autoconf documentation. autoreconf: error: /opt/homebrew/Cellar/autoconf/2.71/bin/autoconf failed with exit status: 1 christopherwensley@Christophers-iMac semigroups % ./configure -with-gaproot=/Users/christopherwensley/gap/gap-4.12.2 configure: error: cannot find required auxiliary files: config.guess config.sub

cdwensley avatar Nov 07 '23 21:11 cdwensley

PR #965 created : approval required to run the tests.

cdwensley avatar Nov 08 '23 12:11 cdwensley

Resolved in v5.5.0 or earlier

james-d-mitchell avatar Apr 07 '25 08:04 james-d-mitchell