First version of caller for libs #104

sebasguts · 2019-05-14T21:05:17Z

This adds a first version for calling all library functions.

Example for the current looks

julia> using Singular

julia> R,(x,) = Singular.PolynomialRing(Singular.QQ, ["x"] )
(Singular Polynomial Ring (QQ),(x),(dp(1),C), spoly{n_Q}[x])

julia> p = 15*x^3+x
15*x^3+x

julia> Singular.LibPoly.normalize(p)
x^3+1/15*x

What is currently in this PR (to be updated)

A C++-level caller (file deps/src/caller.cpp) for library functions, that automatically translates the Julia input into valid Singular input. Currently supported types (all low-level libSingular CxxWrap objects)
- bigint
- bigintmat
- ideal
- int
- intmat
- intvec
- list
- map
- matrix
- module
- number
- poly
- resolution
- ring
- string
- vector
A julia level low_level_caller (file src/libsingular/caller.jl), that calls library functions and translates the output. All high-level Singular.jl types are supported as input (as long as their low-level equivalent is mentioned above). Current supported output is
- bigint
- bigintmat
- ideal
- int
- intmat
- intvec
- list
- map
- matrix
- module
- number
- poly
- resolution
- ring
- string
- vector
A parser that parses all Singular .lib files and creates a julia dictionary. To be hooked into the build script (files deps/libparser.jl and etc/lib_parser.sh).
A meta programming part (file src/libsingular/Meta.jl) that creates a module for each library and adds all global functions from that library. Note here that all this module generation happens at precompilation time. So no time at the normal startup is wasted.

What needs to be done

Support more Singular.jl output types, ideally by making Rings callable on more CxxWrap pointer objects, like for polys:

julia> R,(x,) = Singular.PolynomialRing(Singular.QQ, ["x"] )
(Singular Polynomial Ring (QQ),(x),(dp(1),C), spoly{n_Q}[x])

julia> R(x.ptr)
x

Remove some inefficiencies in the code generated by the metapackage (currently, substitution of symbols into a string does not work as expected).
Provide some test input
Add return dictionary for rings
Add input and output hooks that get executed on the input before calling the function and on the output after calling the functions (for example, to get the right member out of a ring dictionary)
find a suitable way to rewrap ring pointers
Remove the allcaps from library names, probably by prefixing the module name or putting the library modules not directly into singular, but in a separate library module
Add a high level wrapper for singulars bigintmat, or convert them directly to something in Julia

deps/src/caller.cpp

etc/parse_libs.sh

deps/parselibs.jl

src/LibSingular.jl

deps/src/caller.cpp

sebasguts · 2019-06-18T21:08:20Z

Even though this might have some inconsistencies, I think it is ready for review now.

sebasguts · 2019-06-18T21:56:00Z

@hannes14 Can you have a look at the tests failing for the resolutions?

I have fixed most, the other ones are tests for length, which has changed with our new setup. Do we return the length incorrectly?

deps/parselibs.jl

deps/build.jl

deps/parselibs.jl

deps/src/caller.cpp

deps/src/coeffs.cpp

deps/src/singular.cpp

test/caller-test.jl

wbhart

So from what I understand so far, this is a low level interface to the interpreter, plus fix ups for the resolutions (i.e. move some stuff to the C++ side), plus some use of the new CxxWrap tuple return values, plus a way of calling library functions on existing high level objects.

What I am not convinced of is the need to call such functions via Singular.SOMENAMEINCAPSLIKEGAP.function. This doesn't just create a bunch of top-of-module-level Julia functions that apply to the existing high level objects that we had before in Singular.jl. For example, in Singular, when I import a library, those functions get injected into scope and I can just call them on interpreter objects directly. (Or at least I think it does; just checking this with Hans now.)

Is there documentation in the docs of the low level interface and how one can use/extend it?

Thanks very much for the code you have written, by the way. This is enormously helpful.

deps/src/CMakeLists.txt

deps/src/caller.cpp

deps/src/coeffs.cpp

src/Meta.jl

src/Singular.jl

src/module/ModuleTypes.jl

test/caller-test.jl

src/LibSingular.jl

wbhart · 2019-06-19T11:16:41Z

By the way, when you are "using Singular", all the symbols exported in Singular are available at the top level. You don't need to write Singular.PolynomialRing or Singular.QQ, etc. In fact, it is bad practice to do this, because it hides cases where things should have been exported for the user, but have accidentally been omitted from the export list.

In particular, Singular.blah should be blah in all the test code!

test/caller-test.jl

sebasguts · 2019-06-19T13:59:54Z

What I am not convinced of is the need to call such functions via Singular.SOMENAMEINCAPSLIKEGAP.function. This doesn't just create a bunch of top-of-module-level Julia functions that apply to the existing high level objects that we had before in Singular.jl. For example, in Singular, when I import a library, those functions get injected into scope and I can just call them on interpreter objects directly. (Or at least I think it does; just checking this with Hans now.)

I do not really understand that comment can you elaborate on what is bothering you about it in particular?

wbhart · 2019-06-19T16:08:44Z

What I am not convinced of is the need to call such functions via Singular.SOMENAMEINCAPSLIKEGAP.function. This doesn't just create a bunch of top-of-module-level Julia functions that apply to the existing high level objects that we had before in Singular.jl. For example, in Singular, when I import a library, those functions get injected into scope and I can just call them on interpreter objects directly. (Or at least I think it does; just checking this with Hans now.)

I do not really understand that comment can you elaborate on what is bothering you about it in particular?

It's not convenient for the user. Who wants to type all of that? Remember Singular.jl is designed to be used standalone, not just as a backend for something else.

The capital letters really have to be changed. Consider LibPoly instead of POLY, assuming that is what the name of the Singular library is. It's fine to be able to access the functions via this, but the user will not want to do this.

However, let's see if @hannes14 has an opinion on this. Does the Singular interpreter force you to write Poly.blah to call the function blah in the Poly library, or does it just inject the function names so you can just type blah?

Maybe some of the function names already exist because we already implemented them directly via the kernel? I certainly prefer that rather than have them handled via the interpreter. LibPoly sounds like default Singular functionality rather than functionality from a library. I guess I don't mind default Singular functionality being accessible via Interpreter.blah, for the cases where we haven't wrapped the corresponding kernel function. But surely true library functionality should be imported for the user.

src/caller.jl

src/resolution/resolution.jl

sebasguts · 2019-06-19T22:06:17Z

Consider LibPoly instead of POLY, assuming that is what the name of the Singular library is. It's fine to be able to access the functions via this, but the user will not want to do this.

I changed it to LibPoly

Maybe some of the function names already exist because we already implemented them directly via the kernel?

Absolutely not the problem. Ideal is defined as the constructor for an ideal in Singular.jl, but it is also a Library. Some functions have the same name as their library name. This has nothing to do with distinction between kernel and library, but that library names are orthogonal to anything. And Singular users know in which library their stuff lives. Exporting single library functions into the Singular module is possible, but this needs to be done by hand. Currently, putting each library into a different module is the right choice.

src/resolution/resolution.jl

deps/src/coeffs.cpp

etc/parse_libs.sh

src/Meta.jl

src/caller.jl

fingolfin · 2019-06-19T22:26:16Z

src/caller.jl

+    return return_dict
+end
+
+function convert_return_value(single_value, rng = nothing)


Some comments here and elsewhere are badly missing. For starters, what is single_value expected to contain?

fingolfin · 2019-06-19T22:30:24Z

src/caller.jl

+# end
+
+casting_functions_pre = Dict(:NUMBER_CMD     => (libSingular.NUMBER_CMD_CASTER,     true, ()),
+    :RING_CMD       => (libSingular.RING_CMD_CASTER,       false, ()),


A comment explaining the meaning of the entries of those tuples would be nice

src/caller.jl

fingolfin · 2019-06-21T09:38:20Z

src/caller.jl

+end
+
+function prepare_argument(x::Array{Int64,1})
+    return Any[ mapping_types_reversed[:INTVEC_CMD], libSingular.jl_array_to_intvec(x) ], nothing


I find this whole business of generating an array in the C code in function get_type_mapper , exporting this to Julia, then turning it into a "reverse" mapping dictionary mapping_types_reversed there, then using that to convert a Julia symbol to an integer on the Julia level, then passing this back to the kernel, overall somewhat hard to track.

Can't we radically simplify this, by creating that "reverse mapping" dictionary in the kernel (possibly even as a plain C++ map!), and never exporting it from there? Then the above line would become just this:

Suggested change

return Any[ mapping_types_reversed[:INTVEC_CMD], libSingular.jl_array_to_intvec(x) ], nothing

return Any[ :INTVEC_CMD, libSingular.jl_array_to_intvec(x) ], nothing

And in caller.cpp code, you only need to modify translate_singular_type, to expect a julia symbol as first argument, instead of integer. It then only needs to modify that integer.

I believe with this relatively straightforward change, things get quite a bit simpler.

(I missed that also create_casting_functions and friends would need to be updated).

Anyway, thinking some more about this, it would probably be even simpler if indeed some of this stuff was replaced by a program that generates the required data (just as we do in https://github.com/gap-packages/SingularInterface) -- then that C/C++ program could in particular creates a .jl file which simply defines Julia constants with the values of INTVEC_CMD etc.. Then, we can use these everywhere instead of symbols, and completely dispense with the conversions between symbols and ints. Also, Julia could then optimize the code some more, I guess.

Looking at the CxxWrap.jl "documentation" (i.e., its README -- is there anything else?), I guess one could also export the yytokentype enum to Julia. But from the explanations there, it is unclear to me whether one can convert the enum values into an Cint, as we would want.

I think we should open a ticket for this and deal with it later. I doubt Sebastian has time to work on such a thing now.

fingolfin · 2019-06-21T09:41:50Z

src/caller.jl

+function low_level_caller_rng(lib::String, name::String, ring, args...)
+    libSingular.load_library(lib)
+    arguments = [prepare_argument(i) for i in args]
+    arguments = Any[ i for (i, j) in arguments ]


So you throw away the rings here. Is this safe? What will happen if I pass in an argument that lives in a different ring? At least in the past, this could crash Singular, IIRC (but perhaps I am wrong? @hannes14 will know better :-)

Ideally, we would compare rings, yes.

fingolfin · 2019-06-21T09:47:21Z

src/caller.jl

+    rng = nothing
+    for (i, j) in arguments
+        if j != nothing
+            rng = j


So, you take the last ring of all the rings. Why not the first (and then insert a break here)? Or, even better: Why not check that all subsequent rings are compatible with the first -- as I also suggested for other variant of this function. With pseudo code like this:

arguments = [] for i in args i, j = prepare_argument(i) push!(arguments, i) if j != nothing if rng == nothing rng = j else # TODO: check "compatibility, e.g. rng.ptr == j.ptr # (but perhaps that is too restrictive?) end end

With that, the two function variants can be merged, with this ring-free variant becoming just return low_level_caller(lib, name, nothing, args...)

src/ideal/ideal.jl

deps/parselibs.jl

fingolfin · 2019-06-26T13:51:40Z

deps/parselibs.jl

+        if libs.stderr != ""
+            error("from libparse: $(libs.stderr)")
+        end
+        libs_splitted = split(libs.stdout,"\n")[4:end-1]


I think a comment here would be very helpful; something like this:

# The output of libparse looks as follows: each line has the form ... # .... give generic description of the form ... # For a concrete example, it might be this: # ... concrete example ... # From this, we need to extra ...

fingolfin · 2019-06-26T13:55:37Z

deps/parselibs.jl

+        end
+        libs_splitted = split(libs.stdout,"\n")[4:end-1]
+        libs_splitted = [ split(i," ") for i in libs_splitted ]
+        libs_splitted = [ [ j for j in i if j != ""] for i in libs_splitted ]


This is redundant if you add keepempty = false to your call to split

fingolfin · 2019-06-26T13:56:30Z

deps/parselibs.jl

+            error("from libparse: $(libs.stderr)")
+        end
+        libs_splitted = split(libs.stdout,"\n")[4:end-1]
+        libs_splitted = [ split(i," ") for i in libs_splitted ]


The next line becomes redundant if you do this instead:

Suggested change

libs_splitted = [ split(i," ") for i in libs_splitted ]

libs_splitted = [ split(i," ", keepempty = false) for i in libs_splitted ]

Also, it seems you prefer list comprehensions (which do me is very Pythonic), while I prefer classic FP style map/filter/... (probably because it is close to what I'd do in GAP). I have no idea what is considered to be "more typical Julia" or preferred in this project; perhaps it doesn't matter. But just to mention it, I think you could also do this (but this is not a change request):

Suggested change

libs_splitted = [ split(i," ") for i in libs_splitted ]

libs_splitted = map( i-> split(i," ", keepempty = false), libs_splitted ]

fingolfin · 2019-07-24T20:07:40Z

It would be a pity if this work bitrotted. I've thus now rebased this, and squashed it into one commit; then added one commit with a few minor changes that address a few of my own remarks (it can be squashed later, I just wanted to make transparent what I did).

I wonder if perhaps this PR could be split into some more digestible parts; e.g. the syzygy related changes seem as if they might be useful independently of the library caller stuff?

wbhart · 2019-07-24T21:04:03Z

It seems to come close to passing, except for the off by one error in resolutions and a missing function.

If we can get it passing, we should just merge it. Singular.jl is not so mature as to agonise over every PR.

sebasguts · 2019-07-24T21:16:03Z

@fingolfin Your changes are fine. As for the resolutions, we should probably send an email to Hans (I also already tagged him somewhere in some comment) since I have no idea if the changes or the tests are actually wrong. I think it is mostly an off by one error, that is strangely deep down in some Singular data structure. I will send him an email, asking about this.

* Check overflow when converting Julia arrays/matrices to internal Singualr vector structure * Use C-structs for type mapper instead of stl * Added comments * Remove unnecessary `rng_ptr` variable from `prepare_argument` * In Meta.jl, use `get` instead of `&&?:` * Simplified internal library caller code

sebasguts · 2019-07-28T13:35:16Z

I added some comments and pushed some tweaks. More to come later.

wbhart · 2019-09-19T13:33:28Z

I asked @hannes14 about the length failures and it seems we should subtract 1 from the length as in the original code. I've made these changes, which should kill 6 of the 8 test failures.

wbhart · 2019-09-19T13:35:07Z

I propose we add tickets for the remaining three todos and work on fixing those at some later date. People need access to this as soon as possible, so we should get it working rather than letting it bitrot.

codecov · 2019-09-23T19:12:43Z

Codecov Report

Merging #104 into master will not change coverage.
The diff coverage is 0%.

@@          Coverage Diff          @@
##           master   #104   +/-   ##
=====================================
  Coverage       0%     0%           
=====================================
  Files          34     36    +2     
  Lines        2783   2877   +94     
=====================================
- Misses       2783   2877   +94

Impacted Files	Coverage Δ
src/ideal/ideal.jl	`0% <0%> (ø)`	⬆️
src/LibSingular.jl	`0% <0%> (ø)`	⬆️
src/module/module.jl	`0% <0%> (ø)`	⬆️
src/module/ModuleTypes.jl	`0% <0%> (ø)`	⬆️
src/number/NumberTypes.jl	`0% <0%> (ø)`	⬆️
src/poly/poly.jl	`0% <0%> (ø)`	⬆️
src/Singular.jl	`0% <0%> (ø)`	⬆️
src/resolution/ResolutionTypes.jl	`0% <0%> (ø)`	⬆️
src/resolution/resolution.jl	`0% <0%> (ø)`	⬆️
src/Meta.jl	`0% <0%> (ø)`
... and 4 more

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 5184de8...c1f13fc. Read the comment docs.

wbhart · 2019-09-26T13:31:17Z

I'm going to merge this now, as we are about to have some breaking changes in Singular.jl, and I don't fancy rebasing this patch. We can argue about the definition of a resolution later and change it if necessary.

As far as I see, there are just the following changes that need to be added in future:

Additional code comments as requested by @fingolfin
Implement bigintmat
Provide some test input (though I have no idea what this means)
Remove ALLCAPS from library names

I will eventually open tickets for these, as @sebasguts has only volunteered to do the first of these.

First version of caller for libs

fingolfin reviewed May 21, 2019

View reviewed changes

sebasguts force-pushed the caller branch from 9a8014d to 55ad032 Compare June 11, 2019 21:53

hannes14 mentioned this pull request Jun 14, 2019

add radical and prepare for more #103

Closed

sebasguts force-pushed the caller branch from 979c779 to cc47273 Compare June 18, 2019 21:05

sebasguts marked this pull request as ready for review June 18, 2019 21:07

wbhart mentioned this pull request Jun 19, 2019

Oscar Roadmap oscar-system/ComAlg.jl#4

Closed

42 tasks

fingolfin mentioned this pull request Jun 19, 2019

build.jl: refactor how we invoke configure #109

Merged

fingolfin reviewed Jun 19, 2019

View reviewed changes

wbhart suggested changes Jun 19, 2019

View reviewed changes

wbhart reviewed Jun 19, 2019

View reviewed changes

test/caller-test.jl Outdated Show resolved Hide resolved

wbhart reviewed Jun 19, 2019

View reviewed changes

src/LibSingular.jl Outdated Show resolved Hide resolved

wbhart reviewed Jun 19, 2019

View reviewed changes

test/caller-test.jl Outdated Show resolved Hide resolved

fingolfin reviewed Jun 19, 2019

View reviewed changes

src/caller.jl Outdated Show resolved Hide resolved

fingolfin reviewed Jun 19, 2019

View reviewed changes

src/resolution/resolution.jl Outdated Show resolved Hide resolved

fingolfin reviewed Jun 19, 2019

View reviewed changes

src/caller.jl Show resolved Hide resolved

fingolfin reviewed Jun 21, 2019

View reviewed changes

sebasguts mentioned this pull request Jun 24, 2019

Added interface to Singular interpreter #110

Merged

sebasguts force-pushed the caller branch from e5fe0bb to 6d057d3 Compare June 24, 2019 17:05

fingolfin reviewed Jun 26, 2019

View reviewed changes

sebasguts and others added 2 commits July 24, 2019 21:54

First version of caller - WIP

0b5a8c5

fixup! First version of caller - WIP

4f321a2

fingolfin force-pushed the caller branch from 6d057d3 to 4f321a2 Compare July 24, 2019 20:04

wbhart added 3 commits September 18, 2019 16:12

Merge branch 'master' into caller

a5f26cf

Fix bug in length that causes test code to fail.

c244bb7

Fix return type of syMinimize

ce9bbbd

wbhart and others added 11 commits September 19, 2019 16:17

Minor formatting cleanup.

51ee35f

Another minor formatting fix.

6d2add2

Minor text formatting issues.

0d0d355

Minor textual changes.

81d55b2

Minor textual formatting.

7d12206

Typo.

3f9f662

Minor textual fixes.

a572191

Typo.

055033c

Added Ring for test

1e3ac2e

Fixed bug in the low_level_caller without specific ring argument

3d2b89e

Update resolution.jl

c1f13fc

wbhart mentioned this pull request Sep 25, 2019

Oscar Roadmap oscar-system/Oscar.jl#1

Closed

48 tasks

wbhart merged commit af812af into oscar-system:master Sep 26, 2019

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

Merge pull request oscar-system#104 from sebasguts/caller

3501be5

First version of caller for libs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

First version of caller for libs #104

First version of caller for libs #104

sebasguts commented May 14, 2019 •

edited

Loading

sebasguts commented Jun 18, 2019

sebasguts commented Jun 18, 2019

wbhart left a comment •

edited

Loading

wbhart commented Jun 19, 2019 •

edited

Loading

sebasguts commented Jun 19, 2019

wbhart commented Jun 19, 2019

sebasguts commented Jun 19, 2019

fingolfin Jun 19, 2019

fingolfin Jun 19, 2019

fingolfin Jun 21, 2019

fingolfin Jun 21, 2019

fingolfin Jun 21, 2019

wbhart Sep 19, 2019

fingolfin Jun 21, 2019

sebasguts Jul 28, 2019

fingolfin Jun 21, 2019

fingolfin Jun 26, 2019

fingolfin Jun 26, 2019

fingolfin Jun 26, 2019

fingolfin commented Jul 24, 2019

wbhart commented Jul 24, 2019

sebasguts commented Jul 24, 2019

sebasguts commented Jul 28, 2019

wbhart commented Sep 19, 2019 •

edited

Loading

wbhart commented Sep 19, 2019

codecov bot commented Sep 23, 2019 •

edited

Loading

wbhart commented Sep 26, 2019

	return Any[ mapping_types_reversed[:INTVEC_CMD], libSingular.jl_array_to_intvec(x) ], nothing
	return Any[ :INTVEC_CMD, libSingular.jl_array_to_intvec(x) ], nothing

	libs_splitted = [ split(i," ") for i in libs_splitted ]
	libs_splitted = [ split(i," ", keepempty = false) for i in libs_splitted ]

	libs_splitted = [ split(i," ") for i in libs_splitted ]
	libs_splitted = map( i-> split(i," ", keepempty = false), libs_splitted ]

First version of caller for libs #104

First version of caller for libs #104

Conversation

sebasguts commented May 14, 2019 • edited Loading

Example for the current looks

What is currently in this PR (to be updated)

What needs to be done

sebasguts commented Jun 18, 2019

sebasguts commented Jun 18, 2019

wbhart left a comment • edited Loading

Choose a reason for hiding this comment

wbhart commented Jun 19, 2019 • edited Loading

sebasguts commented Jun 19, 2019

wbhart commented Jun 19, 2019

sebasguts commented Jun 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fingolfin commented Jul 24, 2019

wbhart commented Jul 24, 2019

sebasguts commented Jul 24, 2019

sebasguts commented Jul 28, 2019

wbhart commented Sep 19, 2019 • edited Loading

wbhart commented Sep 19, 2019

codecov bot commented Sep 23, 2019 • edited Loading

Codecov Report

wbhart commented Sep 26, 2019

sebasguts commented May 14, 2019 •

edited

Loading

wbhart left a comment •

edited

Loading

wbhart commented Jun 19, 2019 •

edited

Loading

wbhart commented Sep 19, 2019 •

edited

Loading

codecov bot commented Sep 23, 2019 •

edited

Loading