Added interface to Singular interpreter #110

sebasguts · 2019-06-22T19:39:25Z

This functionality was specifically requested by @mohamed-barakat for incrementally migrating his homalg Singular interface to Singular.jl

wbhart · 2019-06-22T19:50:48Z

I will wait for @mohamed-barakat to give his review before merging, to confirm it does what he actually wants. It seems very limited compared to what we will get once the full interpreter interface is there, and to me it just looks like code that will be removed eventually, though perhaps that is the intention.

What does @fingolfin think about this?

sebasguts · 2019-06-22T19:58:33Z

@wbhart Absolutely, very limited, and not particularly useful when using Singular.jl

But this is indeed his intention. Currently, homalg works via passing strings through a terminal interface. This can be used to replace this. I do not really think that this is valid strategy either, on the other hand, having it does not really harm anyway. It is not compatible with the usual way of using Singular via Singular.jl, so it won't harm too much.

deps/src/singular.cpp

test/stringinterface-test.jl

fingolfin · 2019-06-23T21:54:15Z

test/stringinterface-test.jl

+    @test result[2] == "x\n"
+    @test result[3] == ""
+    @test result[4] == ""
+    @test length(result) == 4


I have a feel that this test would be a lot more helpful if it was closer to a GAP style .tst test. Luckily, as I learned recently in Kaiserslautern, this can be done in Julia, too, using a jldoctest (see https://juliadocs.github.io/Documenter.jl/stable/man/doctests/), which at the same time put a nice example into the manual, which is always good, isn't it ? :-)

I would really prefer to not document this function in the manual. It should not be used, as it behaves absolutely orthogonal to the rest of Singular.jl, and might cause the user more headache than it serves them. This is mostly to be used by @mohamed-barakat , and I do not even think that his use-case is useful.

codecov · 2019-06-24T07:00:30Z

Codecov Report

Merging #110 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #110   +/-   ##
=====================================
  Coverage       0%     0%           
=====================================
  Files          30     30           
  Lines        2588   2588           
=====================================
  Misses       2588   2588

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea3e4af...b4868d2. Read the comment docs.

codecov · 2019-06-24T07:00:30Z

Codecov Report

Merging #110 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##           master   #110   +/-   ##
=====================================
  Coverage       0%     0%           
=====================================
  Files          30     30           
  Lines        2591   2591           
=====================================
  Misses       2591   2591

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2402aa...28b4890. Read the comment docs.

fingolfin · 2019-06-24T07:17:09Z

deps/src/singular.cpp

@@ -53,6 +72,45 @@ JLCXX_MODULE define_julia_module(jlcxx::Module & Singular)
    singular_define_ideals(Singular);
    singular_define_matrices(Singular);

+    Singular.method("call_interpreter", [](std::string input) {
+        // set callbacks


actually, shouldn't that be:

Suggested change

// set callbacks

// save callbacks

and then the next three lines have // set callbacks?

deps/src/singular.cpp

fingolfin · 2019-06-24T07:21:27Z

test/call_interpreter-test.jl

+    @test result[2] == "x\n"
+    @test result[3] == ""
+    @test result[4] == ""
+    @test length(result) == 4


Is there any way right now to interact with any of the objects created with these command? E.g. a function which takes the string "f" as input and returns the value assigned to the variable "f" right now?

Or is all interaction really meant to go through string parsing?

Exactly, absolutely only via strings, there is never a leftv or anything returned.

I am actually not sure if we could have something like GAP.jl's EvalString that returns the value a certain command is returning. We could have this by using some dummy library function, but because of Singular's current ring I would actually consider this very hard to correctly use, and dangerous.

We have that in SingularInterface and it is perfectly safe to use there. The function provided by this PR is indeed very unsafe, and hence hard to justify. But given that we have proven that it can be done better, I'd argue that's a flaw in your implementation, not Mohamed's request :-)

I am absolutely okay with not adding this function, or adding a different one instead. But this function does what was requested, as strings by @mohamed-barakat mostly have the form varname = something, and do not need any returning object, but the output string. So this function should do the trick for him.

Main problem with the function you propose is that we currently do not handle leftvs in Singular.jl. So the result would have to be piped through the machinery I introduced in #104 to get something Singular.jl can actually understand. So maybe we should have both functions. You can also view this function as the beginning of a Singular REPL mode, although nobody has (and probably will ever) asked for it.

wbhart · 2019-06-24T15:48:51Z

There seem to be some test failures.

fingolfin

In principle I am OK with this now, modulo the fundamental question of whether it is a good idea to merge this, at least in the current form. For example, it is not clear to me if using this new function to change the current ring and do other stuff might have breaking effects on code using the high(er) level functions in Singular.jl

fingolfin · 2019-06-26T14:04:17Z

deps/src/singular.cpp

+        bool err = iiAllStart(NULL, ost, BT_proc, 0);
+        inerror = 0;
+        errorreported = 0;
+        omFree(ost);


I had a look at the code for iiAllStart. It does exactly one thing with that argument: it passes it to omStrDup :-). So, yeah, I am pretty sure that you can just pass input_str.c_str() to iiAllStart :-)

deps/src/singular.cpp

wbhart · 2019-06-26T14:28:29Z

I am happy to merge it once the tests pass and @mohamed-barakat confirms that this does everything he wants.

sebasguts · 2019-06-26T14:39:13Z

I updated with the changes @fingolfin suggested.

mohamed-barakat · 2019-06-26T15:57:34Z

I'll come to it next week, then I'll confirm.

mohamed-barakat · 2019-06-26T19:37:36Z

How can I access this function from the GAP side, i.e., using JuliaInterface/JuliaExperimental? I assumed it should be something like Julia.Singular.libSingular.call_interpreter but it is not bound.

sebasguts · 2019-06-27T11:54:06Z

@fingolfin could you have a look at the mac failures? I do not understand them tbh.

fingolfin · 2019-06-27T14:31:11Z

@sebasguts will do. as a matter of fact, that's why I've been trying to get singular and Singular.jl to compiler on my Mac for the past 2 days (albeit not non-stop, but rather whenever I had 5 spare minutes here and there)... I hope it'll work now, then I can debug this.

fingolfin · 2019-06-27T14:35:02Z

deps/src/singular.cpp

+
+        // call interpreter
+        std::string input_str = input + "\nreturn();";
+        bool err = iiAllStart(NULL, const_cast<char *>(input_str.c_str()),


I made Singular/Singular#931 so we could simplify this in the future.

sebasguts · 2019-06-27T15:33:12Z

Okay, thanks :). When we can see what is actually wrong on Mac, we can fix this problem. For now, I could add some ifdefs, but that is not really a good idea...

mohamed-barakat · 2019-06-28T11:15:14Z

I am now also unable to build Singular.jl on my Mac.

wbhart · 2019-06-28T12:24:47Z

I believe @fingolfin is currently working on fixing that. I'm not sure what changed, as I'm not a Mac person, but at least @fingolfin has one.

fingolfin · 2019-06-28T17:25:36Z

The problem is that WerrorS_callback are not defined in libsingular, but rather in libsingular_resources. I tried to push a fix to this PR to verify it works, but I lack the permissions (as I am not a member of this repository). So here's the proposed patch as a diff instead:

commit effdafbb5dd89441c6acf3d157392162b36d517a
Author: Max Horn <[email protected]>
Date:   2019-06-28 19:22:31 +0200

    Link against libsingular_resources

diff --git a/deps/src/CMakeLists.txt b/deps/src/CMakeLists.txt
index f1b05a6..acd8e80 100644
--- a/deps/src/CMakeLists.txt
+++ b/deps/src/CMakeLists.txt
@@ -14,7 +14,7 @@ SET( CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++14" )
 SET( CMAKE_SHARED_LINKER_FLAGS  "${CMAKE_SHARED_LINKER_FLAGS} -L${JULIA_LIB_DIR} -Wl,-rpath,${JULIA_LIB_DIR} -L${singular_libdir} -Wl,-rpath,${singular_libdir}" )
 
 add_library(singularwrap SHARED singular.cpp rings.cpp coeffs.cpp ideals.cpp matrices.cpp)
-target_link_libraries(singularwrap JlCxx::cxxwrap_julia -ljulia "-lSingular -lpolys -lfactory -lomalloc -ldl")
+target_link_libraries(singularwrap JlCxx::cxxwrap_julia -ljulia "-lSingular -lpolys -lsingular_resources -lfactory -lomalloc -ldl")
 
 install(TARGETS
   singularwrap

On the long run, it would be better to use libsingular-config to get the correct compiler and linker flags (see also Singular/Singular#933), or the pkg-config file Singular.pc (with help of https://cmake.org/cmake/help/latest/module/FindPkgConfig.html).

mohamed-barakat

I have tested the commit under Linux. It does exactly what I want. The entire homalg project (and hence the CAP project) can now access the Singular in Singular.jl by redefining:

HOMALG_IO_Singular.LaunchCAS :=
function( arg )
    local s;

    s := rec(
             lines := "",
             errors := "",
             SendBlockingToCAS := SendBlockingToCASJSingularInterpreterForHomalg,
             SendBlockingToCAS_original := SendBlockingToCASJSingularInterpreterForHomalg,
             TerminateCAS := function( arg ) end,
             InitializeMacros := InitializeMacros,
             InitializeCASMacros := InitializeSingularMacros,
             setinvol := _Singular_SetInvolution,
             init_string := Concatenation( HOMALG_IO_Singular.init_string, ";option(notWarnSB)" ),
             pid := "of GAP",
             remove_enter := true,
             trim_display := ""
             );

    return s;

end;

SendBlockingToCASJSingularInterpreterForHomalg :=
function( stream, input_string )
    local result;

    result := Julia.Singular.libSingular.call_interpreter( GAPToJulia( input_string ) );

    stream.lines := JuliaToGAP( IsString, result[2] );

    if result[1] then
        stream.errors := Concatenation( "error: ", JuliaToGAP( IsString, result[3] ) );
    else
        stream.errors := "";
    fi;

    stream.warning := JuliaToGAP( IsString, result[4] );

end;

Thanks very much @sebasguts.

wbhart · 2019-06-28T20:28:16Z

Great. So we just need to get the CI tests passing on OSX and we should be right to merge this.

relies on oscar-system/Singular.jl#110

via strings in Singular language

sebasguts · 2019-06-30T11:49:11Z

Thx to @fingolfin, this passes now, and @mohamed-barakat has agreed that this is what he needs. Feel free to merge it.

mohamed-barakat · 2019-06-30T11:50:47Z

Wonderful, thanks @sebasguts and @fingolfin.

…interface Added interface to Singular interpreter

sebasguts requested review from wbhart and mohamed-barakat June 22, 2019 19:39

fingolfin requested changes Jun 23, 2019

View reviewed changes

sebasguts force-pushed the sg/library_string_interface branch from b4868d2 to 993e625 Compare June 24, 2019 07:04

fingolfin reviewed Jun 24, 2019

View reviewed changes

deps/src/singular.cpp Outdated Show resolved Hide resolved

fingolfin reviewed Jun 24, 2019

View reviewed changes

deps/src/singular.cpp Outdated Show resolved Hide resolved

fingolfin reviewed Jun 24, 2019

View reviewed changes

deps/src/singular.cpp Show resolved Hide resolved

fingolfin reviewed Jun 24, 2019

View reviewed changes

sebasguts force-pushed the sg/library_string_interface branch 2 times, most recently from 3e6c5a4 to a625eb7 Compare June 24, 2019 08:07

sebasguts force-pushed the sg/library_string_interface branch from a625eb7 to f8bc183 Compare June 26, 2019 13:32

fingolfin reviewed Jun 26, 2019

View reviewed changes

sebasguts force-pushed the sg/library_string_interface branch from f8bc183 to 17e9a7d Compare June 26, 2019 14:38

fingolfin reviewed Jun 27, 2019

View reviewed changes

mohamed-barakat approved these changes Jun 28, 2019

View reviewed changes

mohamed-barakat added a commit to homalg-project/OscarForHomalg that referenced this pull request Jun 29, 2019

OscarForHomalg initial commit

f15e89e

relies on oscar-system/Singular.jl#110

sebasguts force-pushed the sg/library_string_interface branch from 17e9a7d to 28b4890 Compare June 30, 2019 11:08

Added interface to Singular interpreter

28b4890

via strings in Singular language

fingolfin approved these changes Jun 30, 2019

View reviewed changes

fingolfin merged commit a5aadd3 into oscar-system:master Jun 30, 2019

fingolfin added a commit to fingolfin/Singular.jl that referenced this pull request Jun 6, 2023

Merge pull request oscar-system#110 from sebasguts/sg/library_string_…

5d5406a

…interface Added interface to Singular interpreter

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added interface to Singular interpreter #110

Added interface to Singular interpreter #110

sebasguts commented Jun 22, 2019

wbhart commented Jun 22, 2019

sebasguts commented Jun 22, 2019

fingolfin Jun 23, 2019

sebasguts Jun 24, 2019

codecov bot commented Jun 24, 2019

codecov bot commented Jun 24, 2019 •

edited

Loading

fingolfin Jun 24, 2019

fingolfin Jun 24, 2019

sebasguts Jun 24, 2019

fingolfin Jun 24, 2019 •

edited

Loading

sebasguts Jun 24, 2019

sebasguts Jun 24, 2019

wbhart commented Jun 24, 2019

fingolfin left a comment

fingolfin Jun 26, 2019

wbhart commented Jun 26, 2019

sebasguts commented Jun 26, 2019

mohamed-barakat commented Jun 26, 2019

mohamed-barakat commented Jun 26, 2019

sebasguts commented Jun 27, 2019

fingolfin commented Jun 27, 2019

fingolfin Jun 27, 2019

sebasguts commented Jun 27, 2019

mohamed-barakat commented Jun 28, 2019

wbhart commented Jun 28, 2019

fingolfin commented Jun 28, 2019

mohamed-barakat left a comment •

edited

Loading

wbhart commented Jun 28, 2019

sebasguts commented Jun 30, 2019

mohamed-barakat commented Jun 30, 2019

Added interface to Singular interpreter #110

Added interface to Singular interpreter #110

Conversation

sebasguts commented Jun 22, 2019

wbhart commented Jun 22, 2019

sebasguts commented Jun 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jun 24, 2019

Codecov Report

codecov bot commented Jun 24, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbhart commented Jun 24, 2019

fingolfin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbhart commented Jun 26, 2019

sebasguts commented Jun 26, 2019

mohamed-barakat commented Jun 26, 2019

mohamed-barakat commented Jun 26, 2019

sebasguts commented Jun 27, 2019

fingolfin commented Jun 27, 2019

Choose a reason for hiding this comment

sebasguts commented Jun 27, 2019

mohamed-barakat commented Jun 28, 2019

wbhart commented Jun 28, 2019

fingolfin commented Jun 28, 2019

mohamed-barakat left a comment • edited Loading

Choose a reason for hiding this comment

wbhart commented Jun 28, 2019

sebasguts commented Jun 30, 2019

mohamed-barakat commented Jun 30, 2019

codecov bot commented Jun 24, 2019 •

edited

Loading

fingolfin Jun 24, 2019 •

edited

Loading

mohamed-barakat left a comment •

edited

Loading