Skip to content

Conversation

mkoeppe
Copy link
Contributor

@mkoeppe mkoeppe commented Apr 8, 2023

📚 Description

Adding doctest tags # optional - sage.rings.finite_rings, ...number_field, ...padics etc.

While going through the doctests line by line, I also made the following changes:

  • some coding style fixes in the doctests (such as adding spaces around some operators and after commas, following PEP 8) - improved the readability of them in the HTML format by breaking long lines to avoid having to scroll horizontally
  • improved indentation of some doctest input and output
  • improved the sphinx markup of some docstrings.

The doctest tags are preparation for being able to test parts of sage.rings in a modularized setting, in which not all rings are available. See https://doc.sagemath.org/html/en/developer/packaging_sage_library.html#doctest-only-dependencies

Part of:

📝 Checklist

  • The title is concise, informative, and self-explanatory.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation accordingly.

⌛ Dependencies

The mass edits (adding # optional tags, breaking sage: lines) were made using the following Emacs macros.

(defun sage-copy-optional-annotation ()
  "In a 'sage: ' line of a docstring, copy '# optional' from a previous line and
advance to the end of the next 'sage: ' line or to the end of the current docstring.
If invoked elsewhere, just advance to the end of the next 'sage: ' line."
  (interactive)
  (if (save-excursion (beginning-of-line)
		      (looking-at " *sage:"))
      (multiple-value-bind (tab-stop-list text advance)
	  (save-excursion
	    (previous-line)
	    (end-of-line)
	    (condition-case nil
		(re-search-backward "# *optional.*$")
	      (error
	       (values '(88)  ; alignment point for modularization "# optional"s
		       "# optional - "
		       nil))
	      (:success
	       (values (list (- (match-beginning 0)
				(save-excursion
				  (beginning-of-line) (point))))
		       (match-string-no-properties 0)
		       t))))
	(end-of-line)
	(just-one-space)
	(tab-to-tab-stop)
	(insert text)
	(when advance
	  (re-search-forward "sage: \\|\"\"\"")
	  (end-of-line)))
    (re-search-forward "sage:")
    (end-of-line)))

(defun sage--align-comment (alignment)
  (let ((tab-stop-list alignment))
    (just-one-space 0)
    (tab-to-tab-stop)
    (re-search-forward "sage: \\|\"\"\"")
    (end-of-line)))
  

(defun sage-align-optional-annotation ()
  (interactive)
  (if (save-excursion (beginning-of-line)
		      (looking-at " *sage:"))
      (let ((eol (save-excursion (end-of-line)
				 (point))))
	(beginning-of-line)
	(condition-case nil
	    (re-search-forward "# optional - \\(sage\\|numpy\\|scipy\\|sympy\\|fpylll\\|mpmath\\|pplpy\\|sphinx\\|pexpect\\)" eol)  ; standard packages
	  (:success
	   (goto-char (match-beginning 0))
	   (sage--align-comment '(88 100 120)))  ; 88 = alignment point for modularization "# optional"s
					         ; in sphinx furo style
	  (error
	   (condition-case nil
	       (re-search-forward "# optional" eol)  ; optional packages
	     (:success
	      (goto-char (match-beginning 0))
	      (sage--align-comment '(64)))
	     (error
	      (end-of-line)
	      (re-search-forward "# optional")
	      (end-of-line))))))
    (re-search-forward "sage:")
    (end-of-line)))

(defun sage-newline-keep-comment ()
  (interactive)
  (let ((pt (point))
	(eol (save-excursion (end-of-line)
			     (point))))
    (condition-case nil
	(re-search-forward "\\([^#]*\\)\\(#.*\\)?$" eol)
      (error
       (newline))
      (:success
       (let ((text (match-string 1)))
	 (replace-match (make-string (length text) ? )
			nil nil nil 1)
	 (end-of-line)
	 (newline)
	 (py-indent-line)
	 (insert "....:     ")
	 (save-excursion (insert text)))))))

(with-eval-after-load "python-mode"
  (define-key python-mode-map (kbd "C-M-;") 'sage-copy-optional-annotation)
  (define-key python-mode-map (kbd "C-M-'") 'sage-align-optional-annotation)
  (define-key python-mode-map (kbd "C-M-<return>") 'sage-newline-keep-comment))

(font-lock-add-keywords 'python-mode
			'(("^ *\\(sage: \\|[.][.][.][.]: \\)" 1
			   'font-lock-warning-face prepend)))

@@ -850,6 +850,7 @@ cdef class CachedFunction():
sage: I = P*[x,y]
sage: from sage.misc.sageinspect import sage_getdoc
sage: print(sage_getdoc(I.groebner_basis)) # indirect doctest
WARNING: the enclosing module is marked...
Return the reduced Groebner basis of this ideal.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the warning message. Does it show in the doctesting?

[1, 15, 13, 3, 9, 7, 5, 11]
sage: (mod(22,31)^200).nth_root(200)
sage: (mod(22,31)^200).nth_root(200) # optional - sage.groups
5
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it uses sage.groups.generic; but maybe I should ship this module with sagemath-categories

sage: I = P*[a, b]
sage: a + b in I
sage: I = P * [a, b]
sage: a + b in I # optional - sage.libs.singular
True
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we have feature sage.rings.polynomial instead of sage.libs.singular? I feel uncomfortable with those features sage.libs.xxx since the feature name reflects implementation details.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, the sage.libs.singular gives attribution of this feature to a library...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The above code uses just polynomials (over rationals). So it is obvious that it needs sage.rings.polynomial if such feature exists. But to think of sage.libs.singular, the developer should be well aware of singular is used to implement polynomials in sage. This is an implementation detail, and is not obvious for some developer who has worked only in, say, combinatorics.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Here polynomials are already available, but to compute the inclusion in I needs singular. That is is the point.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused. Don't we need singular just to create P or I?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sage has an implementation for multivariate polynomials using dictionaries. It falls back to it when Singular is not available. The ring and also ideals can be created without Singular. Only when Gröbner bases are needed, for example for ideal membership, Singular is required.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. So the existence of the tag # optional - sage.libs.singular in the code is incomprehensible to those that are not familiar with how sage works internally with polynomials. I think this will become a maintenance burden later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a documentation issue that can be solved by expanding the docstring of the feature to explain what Sage uses the library for

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Essentially we should say that singular is needed if your computation involves Groebner basis. But which computation involves Groebner basis is still not obvious to the uninitiated.

I am okay for now. The problem, if it turns to be real, may be solved later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly, Sage has its own "toy" implementation of Groebner bases; but even that depends on Singular

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 22, 2023

Thanks very much for the review!

SageMath version 10.0.rc0, Release Date: 2023-04-23
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 23, 2023

Easy merge.

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 24, 2023

Test failures seem genuine.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 24, 2023

I agree. I think I have a fix for them already on another branch, I'll check

@kwankyu
Copy link
Collaborator

kwankyu commented Apr 24, 2023

From the linter:

./sage/rings/real_double.pyx:1525:1: W293 blank line contains whitespace

This is apparently not from this PR, but this PR also touches the file.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Apr 24, 2023

This is fixed by #35552, I think.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 14, 2023

Fixed

@vbraun
Copy link
Member

vbraun commented May 20, 2023

pdf docs stil fail. Please do not set back to positive review until you have tested it

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 21, 2023

Now it builds

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 21, 2023

conflicts with #35562 though

@vbraun
Copy link
Member

vbraun commented May 21, 2023

Test fail. Which part of "please do not set back to positive review until you have tested it" was unclear?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented May 21, 2023

"it".

@github-actions
Copy link

Documentation preview for this PR is ready! 🎉
Built with commit: 1fa90d0

@vbraun vbraun merged commit f25807f into sagemath:develop May 28, 2023
Comment on lines -1044 to +1049
sage: P.<e,d,c,b,a> = PolynomialRing(QQ,5,order='lex')
sage: P.<e,d,c,b,a> = PolynomialRing(QQ, 5, order='lex'); P.rename("P")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mkoeppe I don't know what's going on here but this rename doesn't seem to be supported:

$ sage
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 10.0, Release Date: 2023-05-20                    │
│ Using Python 3.11.4. Type "help()" for help.                       │
└────────────────────────────────────────────────────────────────────┘
sage: P.<e,d,c,b,a> = PolynomialRing(QQ, 5, order='lex')
sage: P.rename("P")
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
File /usr/lib/python3.11/site-packages/sage/structure/sage_object.pyx:121, in sage.structure.sage_object.SageObject.rename (build/cythonized/sage/structure/sage_object.c:2154)()
    120 try:
--> 121     self.__custom_name = str(x)
    122 except AttributeError:

AttributeError: 'sage.rings.polynomial.multi_polynomial_libsingular.MPolynomialRing_libsingular' object has no attribute '__custom_name'

During handling of the above exception, another exception occurred:

NotImplementedError                       Traceback (most recent call last)
Cell In[2], line 1
----> 1 P.rename("P")

File /usr/lib/python3.11/site-packages/sage/structure/sage_object.pyx:123, in sage.structure.sage_object.SageObject.rename (build/cythonized/sage/structure/sage_object.c:2203)()
    121             self.__custom_name = str(x)
    122         except AttributeError:
--> 123             raise NotImplementedError("object does not support renaming: %s" % self)
    124 
    125 def reset_name(self):

NotImplementedError: object does not support renaming: Multivariate Polynomial Ring in e, d, c, b, a over Rational Field
sage: 

It's not working for me in 10.1.beta7 either. I don't know what is going on, since I had not seen this error before and this was merged in 10.1.beta1.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #35749 already

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #35749 already

Is that the right PR? That one is 157 commits and mentions nothing of the issue.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the top of the description of #35749:

  • Fixing the handling of file-level # optional tags.
  • Some files were not being doctested; fixing the recently introduced errors in doctests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I thought I was getting crazy. I was trying to test #35977 and getting this error I never saw before but now I could reproduce with old branches.

Now there are two more regressions in singular 4.3.2p3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants