sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.44k stars 480 forks source link

Feature for gfan #33468

Closed mkoeppe closed 2 years ago

mkoeppe commented 2 years ago

As a follow-up on #31296 and part of Meta-ticket #30818, we create Features for gfan executables and use these features in sage.interfaces.gfan.

This makes Sage become fully functional even when not being run from within sage-env (which sets PATH).

To test:

$ venv/bin/python3 -c 'from sage.all import PolynomialRing, QQ; R = PolynomialRing(QQ,["z1","zz1"]); z1,zz1 = R.gens(); print(R.ideal([z1**2*zz1-1,zz1-2]).groebner_fan().maximal_total_degree_of_a_groebner_basis())'
2

Following pycodestyle advice, we replace I by input and we add a deprecation message for it:

     sage: _ = gfan(I='Q[x,y]{x^2-y-1,y^2-xy-2/3}', cmd='bases')
     doctest:...:
     DeprecationWarning: use the option 'input' instead of 'I'
     See https://trac.sagemath.org/33468 for details.

Moreover, the format argument was ignored, so we added a deprecation warning about it:

sage: _ = gfan(input='Q[x,y]{x^2-y-1,y^2-xy-2/3}', cmd='bases', format=True)
doctest:...:
DeprecationWarning: argument `format` is ignored in the code: it is now deprecated. Please update your code without this argument as it will be removed in a later version of SageMath.
See https://trac.sagemath.org/33468 for details.

We also added a deprecation for using the ignored format argument in the gfan method as used here:

sage: R.<x,y> = PolynomialRing(QQ,2)
sage: gf = R.ideal([x^3-y,y^3-x-1]).groebner_fan()
sage: gf.gfan(format=True)
... DeprecationWarning: argument `format` is ignored in the code: it is now deprecated. Please update your code without this argument as it will be removed in a later version of SageMath.
See https://trac.sagemath.org/33468 for details.
  gf.gfan(format=True)
'Q[x,y]\n{{\ny^9-1-y+3*y^3-3*y^6,\nx+1-y^3}\n,\n{\nx^3-y,\ny^3-1-x}\n,\n{\nx^9-1-x,\ny-x^3}\n}\n'

Depends on #31296

CC: @seblabbe

Component: refactoring

Author: Matthias Koeppe, Sébastien Labbé

Branch/Commit: 7fd1d61

Reviewer: Sébastien Labbé, Matthias Koeppe

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

mkoeppe commented 2 years ago

Branch: u/mkoeppe/feature_for_gfan

mkoeppe commented 2 years ago

Commit: b24764b

mkoeppe commented 2 years ago

New commits:

b24764bsrc/sage/features/gfan.py: New, use it in sage.interfaces.gfan
mkoeppe commented 2 years ago

Author: Matthias Koeppe

mkoeppe commented 2 years ago

Description changed:

--- 
+++ 
@@ -1 +1,10 @@
+As a follow-up on #31296 and part of Meta-ticket #30818, we create `Feature`s for gfan executables and use these features in `sage.interfaces.gfan`.

+This makes Sage becomes fully functional even when not being run from within `sage-env` (which sets `PATH`).
+
+To test:
+
+```
+$ venv/bin/python3 -c 'import sage.all; R.<z1,zz1> = PolynomialRing(QQ,2); print(R.ideal([z1^2*zz1-1,zz1-2]).groebner_fan())'
+```
+
mkoeppe commented 2 years ago

Description changed:

--- 
+++ 
@@ -5,6 +5,7 @@
 To test:

-$ venv/bin/python3 -c 'import sage.all; R.<z1,zz1> = PolynomialRing(QQ,2); print(R.ideal([z1^2*zz1-1,zz1-2]).groebner_fan())' +$ venv/bin/python3 -c 'from sage.all import PolynomialRing, QQ; R = PolynomialRing(QQ,["z1","zz1"]); z1,zz1 = R.gens(); print(R.ideal([z1*2zz1-1,zz1-2]).groebner_fan().maximal_total_degree_of_a_groebner_basis())' +2

seblabbe commented 2 years ago
comment:5

from .join_feature import JoinFeature is not used in gfan.py

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from b24764b to e6c61eb

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

e6c61ebsrc/sage/features/gfan.py: Remove unused import
mkoeppe commented 2 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,6 @@
 As a follow-up on #31296 and part of Meta-ticket #30818, we create `Feature`s for gfan executables and use these features in `sage.interfaces.gfan`.

-This makes Sage becomes fully functional even when not being run from within `sage-env` (which sets `PATH`).
+This makes Sage become fully functional even when not being run from within `sage-env` (which sets `PATH`).

 To test:
seblabbe commented 2 years ago

Changed commit from e6c61eb to 25674e4

seblabbe commented 2 years ago

Changed branch from u/mkoeppe/feature_for_gfan to public/33468

seblabbe commented 2 years ago
comment:8

I added a commit on top to add docstring and doctest for the __call__ method (after merging with beta6). It made me realize that the format argument is just ignored in the code of __call__. Should we remove/deprecate it?


New commits:

a091fe3Merge branch 'u/mkoeppe/feature_for_gfan' of trac.sagemath.org:sage into 33468
25674e433468: adding doctest to gfan interface call method
mkoeppe commented 2 years ago
comment:9

Replying to @seblabbe:

I added a commit on top to add docstring and doctest for the __call__ method (after merging with beta6).

Thank you!

It made me realize that the format argument is just ignored in the code of __call__. Should we remove/deprecate it?

Good idea, let's deprecate it

mkoeppe commented 2 years ago

Changed author from Matthias Koeppe to Matthias Koeppe, Sébastien Labbé

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

e6e3a3833468: deprecate format argument
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 25674e4 to e6e3a38

seblabbe commented 2 years ago
comment:11

Replying to @mkoeppe:

Good idea, let's deprecate it

Done.

mkoeppe commented 2 years ago
comment:12
$ ./sage -tox -e pycodestyle src/sage/interfaces/gfan.py 
pycodestyle installed: pycodestyle==2.8.0
pycodestyle run-test-pre: PYTHONHASHSEED='1743415609'
pycodestyle run-test: commands[0] | pycodestyle sage/interfaces/gfan.py
sage/interfaces/gfan.py:28:1: E265 block comment should start with '# '
sage/interfaces/gfan.py:41:1: E265 block comment should start with '# '
sage/interfaces/gfan.py:52:24: E741 ambiguous variable name 'I'
sage/interfaces/gfan.py:85:12: E714 test for object identity should be 'is not'
sage/interfaces/gfan.py:88:21: E128 continuation line under-indented for visual indent
sage/interfaces/gfan.py:89:21: E128 continuation line under-indented for visual indent
sage/interfaces/gfan.py:90:21: E128 continuation line under-indented for visual indent
sage/interfaces/gfan.py:114:1: E305 expected 2 blank lines after class or function definition, found 1
3       E128 continuation line under-indented for visual indent
2       E265 block comment should start with '# '
1       E305 expected 2 blank lines after class or function definition, found 1
1       E714 test for object identity should be 'is not'
1       E741 ambiguous variable name 'I'
seblabbe commented 2 years ago
comment:13

hahaha, more comments than lines of code added!

Ok, I will check this out tomorrow.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from e6e3a38 to eed35ab

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

eed35ab33468: fixing pycodestyle warnings
seblabbe commented 2 years ago
comment:15

After sage -pip intall tox, I now get:

$ ./sage -tox -e pycodestyle src/sage/interfaces/gfan.py
pycodestyle installed: pycodestyle==2.8.0
pycodestyle run-test-pre: PYTHONHASHSEED='1348994557'
pycodestyle run-test: commands[0] | pycodestyle sage/interfaces/gfan.py
______________________________________________ summary ______________________________________________
  pycodestyle: commands succeeded
  congratulations :)

Needs review.

mkoeppe commented 2 years ago
comment:16

I don't think we can rename I to input - it is a breaking interface change, as gfan(I=...) will not work any more.

seblabbe commented 2 years ago
comment:17

True. I did that to fix one of the pycodestyle warning E741 ambiguous variable name 'I'.

Either I revert that change or I use @rename_keword(I=input). Any preference?

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from eed35ab to 1a860db

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

1a860db33468: @rename_keyword(I='input')
seblabbe commented 2 years ago
comment:19

So, I added a commit using the rename keyword stuff.

Also, I checked that sage -grep "gfan(I=" does not return any use case in the sage code.

mkoeppe commented 2 years ago
comment:20

I'm getting deprecation warnings in the doctests

File "src/sage/rings/polynomial/groebner_fan.py", line 979, in sage.rings.polynomial.groebner_fan.GroebnerFan.weight_vectors
Failed example:
    g.weight_vectors()
Expected:
    [(3, 7, 1), (5, 1, 2), (7, 1, 4), (5, 1, 4), (1, 1, 1), (1, 4, 8), (1, 4, 10)]
Got:
    doctest:warning
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/bin/sage-runtests", line 149, in <module>
        err = DC.run()
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/control.py", line 1343, in run
        self.run_doctests()
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/control.py", line 1044, in run_doctests
        self.dispatcher.dispatch()
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 2016, in dispatch
        self.parallel_dispatch()
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 1911, in parallel_dispatch
        w.start()  # This might take some time
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 2185, in start
        super(DocTestWorker, self).start()
      File "/usr/local/Cellar/python@3.10/3.10.2/Frameworks/Python.framework/Versions/3.10/lib/python3.10/multiprocessing/process.py", line 121, in start
        self._popen = self._Popen(self)
      File "/usr/local/Cellar/python@3.10/3.10.2/Frameworks/Python.framework/Versions/3.10/lib/python3.10/multiprocessing/context.py", line 224, in _Popen
        return _default_context.get_context().Process._Popen(process_obj)
      File "/usr/local/Cellar/python@3.10/3.10.2/Frameworks/Python.framework/Versions/3.10/lib/python3.10/multiprocessing/context.py", line 277, in _Popen
        return Popen(process_obj)
      File "/usr/local/Cellar/python@3.10/3.10.2/Frameworks/Python.framework/Versions/3.10/lib/python3.10/multiprocessing/popen_fork.py", line 19, in __init__
        self._launch(process_obj)
      File "/usr/local/Cellar/python@3.10/3.10.2/Frameworks/Python.framework/Versions/3.10/lib/python3.10/multiprocessing/popen_fork.py", line 71, in _launch
        code = process_obj._bootstrap(parent_sentinel=child_r)
      File "/usr/local/Cellar/python@3.10/3.10.2/Frameworks/Python.framework/Versions/3.10/lib/python3.10/multiprocessing/process.py", line 315, in _bootstrap
        self.run()
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 2157, in run
        task(self.options, self.outtmpfile, msgpipe, self.result_queue)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 2487, in __call__
        doctests, extras = self._run(runner, options, results)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 2539, in _run
        result = runner.run(test)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/doctest/forker.py", line 866, in run
        return self._run(test, compileflags, out)
      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.rings.polynomial.groebner_fan.GroebnerFan.weight_vectors[2]>", line 1, in <module>
        g.weight_vectors()
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/rings/polynomial/groebner_fan.py", line 988, in weight_vectors
        ans, err = gfan_processes.communicate(input=str_to_bytes(self.gfan()))
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/rings/polynomial/groebner_fan.py", line 1130, in gfan
        s = gfan(I, cmd, verbose=self.__verbose, format=format)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/misc/decorators.py", line 651, in wrapper
        return func(*args, **kwds)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/interfaces/gfan.py", line 96, in __call__
        deprecation(33468, 'argument `format` is ignored in the code: '
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/misc/superseded.py", line 99, in deprecation
        warning(trac_number, message, DeprecationWarning, stacklevel)
      File "/Users/mkoeppe/s/sage/sage-rebasing/worktree-gcc11/src/sage/misc/superseded.py", line 180, in warning
        warn(message, warning_class, stacklevel)
      File "/usr/local/Cellar/python@3.10/3.10.2/Frameworks/Python.framework/Versions/3.10/lib/python3.10/warnings.py", line 109, in _showwarnmsg
        sw(msg.message, msg.category, msg.filename, msg.lineno,
    :
    DeprecationWarning: argument `format` is ignored in the code: it is now deprecated. Please update your code without this argument as it will be removed in a later version of SageMath.
    See https://trac.sagemath.org/33468 for details.
    [(3, 7, 1), (5, 1, 2), (7, 1, 4), (5, 1, 4), (1, 1, 1), (1, 4, 8), (1, 4, 10)]
seblabbe commented 2 years ago
comment:22

Indeed, the patchbot says the following doctests are failing:

----------------------------------------------------------------------
sage -t --long --random-seed=201618586090654190254950585468856090968 src/sage/rings/polynomial/multi_polynomial_ideal.py  # 1 doctest failed
sage -t --long --random-seed=201618586090654190254950585468856090968 src/sage/rings/polynomial/groebner_fan.py  # 3 doctests failed
sage -t --long --random-seed=201618586090654190254950585468856090968 src/doc/ru/tutorial/tour_advanced.rst  # 1 doctest failed
sage -t --long --random-seed=201618586090654190254950585468856090968 src/doc/en/tutorial/tour_advanced.rst  # 1 doctest failed
sage -t --long --random-seed=201618586090654190254950585468856090968 src/doc/fr/tutorial/tour_advanced.rst  # 1 doctest failed
sage -t --long --random-seed=201618586090654190254950585468856090968 src/doc/pt/tutorial/tour_advanced.rst  # 1 doctest failed
sage -t --long --random-seed=201618586090654190254950585468856090968 src/doc/ja/tutorial/tour_advanced.rst  # 1 doctest failed
sage -t --long --random-seed=201618586090654190254950585468856090968 src/doc/de/tutorial/tour_advanced.rst  # 1 doctest failed
----------------------------------------------------------------------
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

c415f70src/sage/features/gfan.py: New, use it in sage.interfaces.gfan
e3df154src/sage/features/gfan.py: Remove unused import
fc0ff5133468: adding doctest to gfan interface call method
b86f85b33468: deprecate format argument
a47a6d233468: fixing pycodestyle warnings
bab4d8a33468: @rename_keyword(I='input')
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 1a860db to bab4d8a

seblabbe commented 2 years ago
comment:24

I rebased on beta7.

seblabbe commented 2 years ago
comment:25

updated description with new added deprecation warnings

seblabbe commented 2 years ago

Description changed:

--- 
+++ 
@@ -9,3 +9,32 @@
 2

+Following pycodestyle advice, we replace I by input and we add a deprecation message for it: + +```

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

7fd1d6133468: deprecating argument format in groebner_fan.py file
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from bab4d8a to 7fd1d61

seblabbe commented 2 years ago
comment:27

I added a deprecation of the format argument in groebner_fan.py file. I checked that it fixes the issues listed in comment:22.

Needs review!

mkoeppe commented 2 years ago

Reviewer: ..., Matthias Koeppe

mkoeppe commented 2 years ago
comment:28

Thanks for all your work on this ticket! LGTM. The patchbot failures are unrelated.

tscrim commented 2 years ago
comment:29

Is comment:28 suppose to be a positive review?

seblabbe commented 2 years ago
comment:30

I think Matthias was waiting for someone to review his commit.

I give a positive review to Matthias' commits.

seblabbe commented 2 years ago

Changed reviewer from ..., Matthias Koeppe to Sébastien Labbé, Matthias Koeppe

mkoeppe commented 2 years ago
comment:31

Thank you!

vbraun commented 2 years ago

Changed branch from public/33468 to 7fd1d61