Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sage.rings: Reformat doctests, add # optional annotations #35457

Merged
merged 52 commits into from
May 28, 2023

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)))

@mkoeppe mkoeppe self-assigned this Apr 9, 2023
@mkoeppe mkoeppe changed the title sage.rings: Add # optional doctest annotations sage.rings: Reformat doctests, add # optional annotations Apr 9, 2023
@mkoeppe mkoeppe force-pushed the sage_rings_optional_annotations branch from 025f9fe to d9fb7be Compare April 15, 2023 08:08
@mkoeppe mkoeppe force-pushed the sage_rings_optional_annotations branch from d9fb7be to 7c692b9 Compare April 15, 2023 08:22
@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
@mkoeppe mkoeppe added this to the sage-10.1 milestone 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.

@mkoeppe mkoeppe deleted the sage_rings_optional_annotations branch July 21, 2023 21:28
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