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.modules.fg_pid.fgp_module: Rename a test_... function to _test_... (with deprecation) #33617

Closed
mkoeppe opened this issue Apr 1, 2022 · 13 comments

Comments

@mkoeppe
Copy link
Member

mkoeppe commented Apr 1, 2022

(split out from #33549, similar to #33612)

In this way we suppress generating the documentation of this undocumented function. https://doc.sagemath.org/html/en/reference/modules/sage/modules/fg_pid/fgp_module.html#sage.modules.fg_pid.fgp_module.test_morphism_0

While at it, we improve the markup of the documentation.

CC: @tscrim @tobiasdiez @simonbrandhorst @jhpalmieri

Component: documentation

Author: Tobias Diez, Matthias Koeppe

Branch/Commit: 66594c2

Reviewer: John Palmieri

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

@mkoeppe mkoeppe added this to the sage-9.6 milestone Apr 1, 2022
@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 1, 2022

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 1, 2022

Commit: 66594c2

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 1, 2022

Author: Matthias Koeppe

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 1, 2022

New commits:

6e70cb2Make test methods private
a0e04ebsrc/sage/modules/fg_pid/fgp_module.py: Add deprecated_function_alias test_morphism_0
66594c2src/sage/modules/fg_pid/fgp_module.py: Fix doc markup

@tscrim
Copy link
Collaborator

tscrim commented Apr 1, 2022

comment:4

For this one, I think we should move this to a method of the corresponding morphism class so we can test it on instances by TestSuite(phi).run(). Thus, the doctest would become

-sage: set_random_seed(s); v = [fgp.test_morphism_0(1) for _ in range(30)]
+sage: set_random_seed(s)
+sage: for _ in range(30):
+....:     phi = random_fgp_morphism_m(1)
+....:     phi.test_morphism_0()

This would also mean a bit other internal changes to the method, but then it can be applied more broadly. It would also be good to have a better name for this test too. Maybe _test_consistency()?

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 1, 2022

Changed author from Matthias Koeppe to Tobias Diez, Matthias Koeppe

@mkoeppe mkoeppe modified the milestones: sage-9.6, sage-9.7 Apr 11, 2022
@jhpalmieri
Copy link
Member

comment:8

Looks okay to me, but what about tscrim's comment:4?

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 15, 2022

comment:9

No idea what these tests do, which is why I cc'ed original authors...

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri

@jhpalmieri
Copy link
Member

comment:10

I think we should merge this now and do the restructuring later.

@mkoeppe
Copy link
Member Author

mkoeppe commented Apr 21, 2022

comment:11

Thanks!

@vbraun
Copy link
Member

vbraun commented May 24, 2022

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

No branches or pull requests

4 participants