variety() for polynomial systems over ℚ using msolve

[mezzarobba]

Add the option of using the external package msolve for computing the variety of a polynomial system. Only systems over ℚ are supported at the moment, because I cannot make sense of msolve's output in the positive characteristic case.

Note that this ticket does not add msolve as a dependency—see #31664 for that.

Upstream: Reported upstream. Developers acknowledge bug.

Component: commutative algebra

Author: Marc Mezzarobba

Branch/Commit: 0312ac6

Reviewer: Sébastien Labbé, Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/33734

[mezzarobba]

New commits:

384780b	#33734 variety(): prepare for multiple algorithm options
ae6c4bf	#33734 variety() over ℚ using msolve

[mezzarobba]

Commit: ae6c4bf

[mezzarobba]

Branch: u/mmezzarobba/33734-msolve

[sheerluck]

comment:2

8 lines of triangLfak part uses 2 space identation, 4 spaces are preferred

[sagetrac-git]

Changed commit from ae6c4bf to 10ae301

[sagetrac-git]

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

e5f0063	#33734 variety(): prepare for multiple algorithm options
10ae301	#33734 variety() over ℚ using msolve

[mezzarobba]

comment:4

Replying to @sheerluck:

8 lines of triangLfak part uses 2 space identation, 4 spaces are preferred

Oops, rebasing mistake (I guess). Thanks!

[sagetrac-git]

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

567cb91

#33734 variety() over ℚ using msolve

[sagetrac-git]

Changed commit from 10ae301 to 567cb91

[sagetrac-git]

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

6ff1148

#33734 variety() over ℚ using msolve

[sagetrac-git]

Changed commit from 567cb91 to 6ff1148

[seblabbe]

comment:7

I think it is preferable to add within this ticket a file sage/features/msolve.py which would check the presence (and functionality) of msolve. You can check other similar files in sage/features which are based on a single command available in PATH. For instance, check at sage/features/pdf2svg.py which also has a spkg= since such a dummy sage package exists, but this is not mandatory, see for instance sage/features/dvipng.py which I think you can essentially copy paste.

If you want, you may also add a is_functional method. See sage/features/latex.py and sage/features/lrs.py for such examples. The is_functional method is not to be used from outside: it is automatically called by is_present() and by require(), see #33114.

The following lines

+    except FileNotFoundError:
+        raise RuntimeError(
+            "could not find the msolve binary. Please install msolve "
+            "(https://msolve.lip6.fr/) and try again.")

will get replaced by

from sage.features.msolve import msolve
msolve.require()

before you use the msolve command.

Also, this will automatically select the optional tag msolve when doctesting the new added file (provided you define the all_features() function at the end of the file sage/features/msolve.py).

[sagetrac-git]

Changed commit from 6ff1148 to bd6bb36

[sagetrac-git]

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

bd6bb36

#33734 sage.features.msolve

[mezzarobba]

comment:9

Replying to @seblabbe:

I think it is preferable to add within this ticket a file sage/features/msolve.py which would check the presence (and functionality) of msolve.

Makes sense; thanks for the pointers!

[seblabbe]

comment:10

Few comments on the code:

1. Are you sure you need all the following imports in the header of the file? I try to keep in the header only the imports that are needed more than once in the file.

+import os
+import tempfile
+import subprocess
+
+import sage.structure.proof.proof
+
+from sage.features.msolve import msolve
+from sage.misc.all import SAGE_TMP
+from sage.misc.converting_dict import KeyConvertingDict
+from sage.misc.sage_eval import sage_eval
+from sage.rings.polynomial.polynomial_ring_constructor import PolynomialRing
+from sage.rings.rational_field import QQ
+from sage.rings.real_arb import RealBallField
+from sage.rings.real_double import RealDoubleField_class
+from sage.rings.real_mpfr import RealField_class
+from sage.rings.real_mpfi import RealIntervalField_class, RealIntervalField

Maybe few could be moved into where they are needed, for instance sage_eval, SAGE_TMP and KeyConvertingDict. Also RealDoubleField_class, RealField_class and RealIntervalField_class.

I would maybe move import subprocess into the method also in the sage/features/msolve.py.

2. Now that the except clause is gone. It is weird to have a try with no except:

+    try:
+        print(",".join(drlpolring.variable_names()), file=msolve_in)
+        print(base.characteristic(), file=msolve_in)
+        print(*(pol._repr_().replace(" ", "") for pol in polys),
+                sep=',\n', file=msolve_in)
+        msolve_in.close()
+        msolve_out = subprocess.run(command, capture_output=True)
+    finally:
+        os.unlink(msolve_in.name)

3. You write .decode('ascii') here:

data = sage_eval(msolve_out.stdout[:-2].decode('ascii'))

Maybe instead, you can use the option text=True in the call to subprocess.run?

4. Why is the line os.unlink(msolve_in.name) needed even after closing the file?

[mezzarobba]

comment:11

Thanks for your comments.

Replying to @seblabbe:

1. Are you sure you need all the following imports in the header of the file? I try to keep in the header only the imports that are needed more than once in the file.

I understand why you may want to do that with the huge source files with functions using widely different tools that we often have in Sage. But what would be the benefit for this module where there is a single function, and other functions we might want to add are likely to need more or less the same imports?

2. Now that the except clause is gone. It is weird to have a try with no except:

Isn't that the standard way of cleaning up after an operation that may raise an error or be interrupted?

3. You write .decode('ascii') here:

data = sage_eval(msolve_out.stdout[:-2].decode('ascii'))

Maybe instead, you can use the option text=True in the call to subprocess.run?

Maybe. As far as I understand msolve will always feed us pure ascii text with unix newlines, so decode('ascii') felt more natural to me, but I'm fine with changing it if there is a reason to do so.

4. Why is the line os.unlink(msolve_in.name) needed even after closing the file?

I am not sure I understand the question. I suppose I could write

    with tempfile.NamedTemporaryFile(dir=SAGE_TMP, mode='w',
                                     encoding='ascii') as msolve_in:
        command = ["msolve", "-f", msolve_in.name]
        if isinstance(ring, (RealIntervalField_class, RealBallField,
                            RealField_class, RealDoubleField_class)):
            parameterization = False
            command += ["-p", str(ring.precision())]
        else:
            parameterization = True
            command += ["-P", "1"]
        print(",".join(drlpolring.variable_names()), file=msolve_in)
        print(base.characteristic(), file=msolve_in)
        print(*(pol._repr_().replace(" ", "") for pol in polys),
                sep=',\n', file=msolve_in, flush=True)
        msolve_out = subprocess.run(command, capture_output=True)

but (1) I find that closing the file is clearer than putting a flush() in the right place, and (2) I need msolve to be able to open the file, which, according to the Python docs, is not allowed on all platforms if Sage keeps it open. So I create the tempfile with delete=False, and hence I am responsible for deleting it.

[seblabbe]

comment:12

Thanks for your answers. I reply below.

Replying to @mezzarobba:

Thanks for your comments.

Replying to @seblabbe:

1. Are you sure you need all the following imports in the header of the file? I try to keep in the header only the imports that are needed more than once in the file.

I understand why you may want to do that with the huge source files with functions using widely different tools that we often have in Sage. But what would be the benefit for this module where there is a single function, and other functions we might want to add are likely to need more or less the same imports?

By experience, sometimes the import in the module creates dependencies just for importing the module which breaks a package which would work (except some parts) if those imports were local imports. But this is when developping an external package. In Sage, this will be fixed anyway. But, I don't know, in my case, I try keep this habit. But, it is true it might not be a sage convention.

Another reason which might be less pointless is to avoid to cluter the tab completion with lots of technical stuff when we do from sage.rings.polynomial.msolve import [TAB].

2. Now that the except clause is gone. It is weird to have a try with no except:

Isn't that the standard way of cleaning up after an operation that may raise an error or be interrupted?

Oups, you are right. I am learning something here. The print(2) is executed here before the exception is raised:

sage: try:
....:     raise Exception
....: finally:
....:     print(2)
....:     
2
Traceback (most recent call last):
...
Exception: 

Whereas below it is not:

sage: raise Exception; print(2)
Traceback (most recent call last):
...
Exception: 

So, I now see the point of the try with no except.

3. You write .decode('ascii') here:

data = sage_eval(msolve_out.stdout[:-2].decode('ascii'))

Maybe instead, you can use the option text=True in the call to subprocess.run?

Maybe. As far as I understand msolve will always feed us pure ascii text with unix newlines, so decode('ascii') felt more natural to me, but I'm fine with changing it if there is a reason to do so.

Well, I think you write .decode('ascii') because it returns the stdout as binary. I am just saying that if you write text=True in the run command, the stdout will already be translated into ascii. So you will not need to translate the binary stdout to to ascii.

4. Why is the line os.unlink(msolve_in.name) needed even after closing the file?

I am not sure I understand the question. I suppose I could write

    with tempfile.NamedTemporaryFile(dir=SAGE_TMP, mode='w',
                                     encoding='ascii') as msolve_in:
        command = ["msolve", "-f", msolve_in.name]
        if isinstance(ring, (RealIntervalField_class, RealBallField,
                            RealField_class, RealDoubleField_class)):
            parameterization = False
            command += ["-p", str(ring.precision())]
        else:
            parameterization = True
            command += ["-P", "1"]
        print(",".join(drlpolring.variable_names()), file=msolve_in)
        print(base.characteristic(), file=msolve_in)
        print(*(pol._repr_().replace(" ", "") for pol in polys),
                sep=',\n', file=msolve_in, flush=True)
        msolve_out = subprocess.run(command, capture_output=True)

but (1) I find that closing the file is clearer than putting a flush() in the right place, and (2) I need msolve to be able to open the file, which, according to the Python docs, is not allowed on all platforms if Sage keeps it open. So I create the tempfile with delete=False, and hence I am responsible for deleting it.

Ok I see.

[sagetrac-git]

Changed commit from bd6bb36 to c7bb8aa

[sagetrac-git]

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

c7bb8aa

#33734 minor style change following reviewer comment

[mezzarobba]

comment:14

Replying to @seblabbe:

By experience, sometimes the import in the module creates dependencies just for importing the module which breaks a package which would work (except some parts) if those imports were local imports.

Sure, but again, this does not apply here. I agree however that it makes sense not to import msolve at the top of polynomial_ideal but only when someone calls a function with algorithm="msolve" (and that's what my code is doing).

Well, I think you write .decode('ascii') because it returns the stdout as binary. I am just saying that if you write text=True in the run command, the stdout will already be translated into ascii. So you will not need to translate the binary stdout to to ascii.

I still don't see the point, but I don't think it can hurt either, so fine, let's use text=True.

[dimpase]

Upstream: Reported upstream. Developers acknowledge bug.

[dimpase]

comment:15

upsteam promises a docs update next week

[mezzarobba]

comment:16

Replying to @dimpase:

upsteam promises a docs update next week

I assume you are taking about the comment I left about the positive characteristic case? If yes, that's indeed what Mohab replied after I reported the issue, but it does not impact this ticket.

(What does impact it a little is that, even in characteristic zero, the descriptions in the msolve tutorial and in msolve --help do not exactly match the output actually produced by the program, so I had to guess some details.)

[dimpase]

comment:17

lgtm

(yes, I meant positive char msolve docs)

[dimpase]

Reviewer: Dima Pasechnik

[mezzarobba]

comment:18

thanks!

[fchapoton]

comment:19

pyflakes plugin is not happy

[sagetrac-git]

Changed commit from c7bb8aa to 0312ac6

[sagetrac-git]

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

4a76ffb	#33734 variety(): prepare for multiple algorithm options
dc858c5	#33734 variety() over ℚ using msolve
43f9458	#33734 sage.features.msolve
0312ac6	#33734 minor style change following reviewer comment

[mezzarobba]

comment:21

Replying to @fchapoton:

pyflakes plugin is not happy

fixed, thanks

[mezzarobba]

Changed reviewer from Dima Pasechnik to Sébastien Labbé, Dima Pasechnik

[vbraun]

Changed branch from u/mmezzarobba/33734-msolve to 0312ac6