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

Modernize coercion for SpecialHyperellipticQuotientRing #33576

Closed
tscrim opened this issue Mar 27, 2022 · 26 comments
Closed

Modernize coercion for SpecialHyperellipticQuotientRing #33576

tscrim opened this issue Mar 27, 2022 · 26 comments

Comments

@tscrim
Copy link
Collaborator

tscrim commented Mar 27, 2022

This is still using the old coercion system and was not covered by the change in #33525.

Depends on #33525

CC: @fchapoton

Component: coercion

Author: Travis Scrimshaw

Branch/Commit: 6f453c1

Reviewer: Frédéric Chapoton

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

@tscrim tscrim added this to the sage-9.6 milestone Mar 27, 2022
@fchapoton
Copy link
Contributor

comment:1

indeed, my fault. I somehow managed to fix only half of the file, sorry.

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 27, 2022

comment:2

I am out of time right now, but I have gotten it down to 21 failures that are all likely from the same root 2 causes. One is needing a coercion, the other is something else that I don't understand at all.

I made a few other changes to try and improve the file, which might have broken things. I did find "one" bug with the cubic quotient one() not having coefficients in the right parent.

Hopefully you can find what is going wrong. I will take a look again later in the week.


New commits:

d32b914tentative to convert monsky to coercion system
c808d47monsky : modern coercion
5abbdf2adding doc
c2c5114various changes in monsky after reviewer's suggestions
cbf7db6one little detail
6cae3dafix doctests
70362aedetails in doc
d08f968Convert MonskyWashnitzerDifferential to use the new coercion model.
dbcb150Modernization of special hyperelliptic quotient and other fixes.

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 27, 2022

Commit: dbcb150

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 27, 2022

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 27, 2022

Author: Travis Scrimshaw

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

Changed commit from dbcb150 to 276d9d2

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

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

276d9d2partially repairing monsky

@fchapoton
Copy link
Contributor

comment:4

down to 2 failing doctests

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 27, 2022

comment:5

Thank you. That was a dumb thing on my part to return the element and not the parent. I will investigate the remaining doctest failures tomorrow morning.

@fchapoton
Copy link
Contributor

comment:6

I think one should exclude _test_divides, _test_zero from the TestSuite.

That would fix one of the two failing doctests.

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 27, 2022

comment:7

I think the _test_zero should be fixed as it is an indication of a bigger underlying bug:

sage: HQR.zero()
0
sage: bool(HQR.zero())
True
sage: f = HQR.zero()._f; f
0
sage: type(f)
<class 'sage.rings.polynomial.polynomial_ring.PolynomialRing_cdvf_with_category.element_class'>
sage: bool(f)
True
sage: F = f.parent()
sage: f == F.zero()
True
sage: f != F.zero()  # it is consistent
False
sage: bool(F.zero())  # but not here
False

My guess for the second failure is we are not converting some element to the base ring of the polynomial ring like we should. Possibly from skipping some checks.

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 27, 2022

comment:8

Indeed, here is the underlying issue:

sage: f.list(copy=False)
[0]
sage: F.zero().list(copy=False)
[]

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

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

98d0709Fixing bug with not normalizing input.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 27, 2022

Changed commit from 276d9d2 to 98d0709

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 27, 2022

comment:10

Indeed, it is as I expected. I have fixed the issues. Now all tests pass for the file.

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@fchapoton
Copy link
Contributor

comment:11

some pyflakes errors:

+src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py:3114:9 local variable 'n' is assigned to but never used
+src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py:3117:9 local variable 'br_zero' is assigned to but never used

Once fixed, you can set to positive on my behalf.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 28, 2022

Changed commit from 98d0709 to 7c47495

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 28, 2022

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

25ba447Merge branch 'develop' into public/coercion/special_hyperelliptic_quotient_ring-33576
7c47495Fixing pyflakes issues in monsky_washnitzer.py.

@tscrim
Copy link
Collaborator Author

tscrim commented Mar 28, 2022

comment:13

Done. Thank you.

@vbraun
Copy link
Member

vbraun commented Mar 31, 2022

comment:14

Merge failure on top of:

a79b1b8 Trac #33573: extend method cores to accept graphs with multiple edges

9543d07 Trac #33570: Add CONTRIBUTING.md to the repository

86d9846 Trac #33569: Speed up methods allow_multiple_edges and multiple_edges

08a9423 Trac #33507: sagemath_doc_html: Use jupyter-sphinx for 3D graphics

3dd4e6b Trac #33568: Add method is_integral_domain for polynomial quotient rings

76d0d33 Trac #33567: finer category for cubic braid group quotients

328de7a Trac #33564: add one test in TestSuite of Coxeter groups

08854a8 Trac #33559: Fix bug in sage.graphs.graph_coloring.vertex_coloring

1762a6c Trac #33558: get rid of __nonzero__ and use __bool__ instead

fbc8bd0 Trac #33555: refresh the file subword.py

42a088a Trac #33553: README Build from Source: m4 (step 5) is needed to run make configure (step 3)

ce74d32 Trac #33540: remove class inheritance of object in combinat

d8f2ce9 Trac #33538: More doctests fix in sage_docbuild

a5446db Trac #33536: Some changes suggested by lgtm

b9cdb7c Trac #33529: Remove old thebe package

211da0e Trac #33214: Vélu isogeny formulas use incorrect a-invariants when pre-isomorphism is set

64bd36e Trac #32687: error in height difference bound

f6ce410 Trac #8784: remove quit_sage() command from all.py top level

3c6f508 Trac #33585: doctest failure when doc html not built/installed but sphinx is available in PYTHONPATH

afa0d96 Trac #33584: Adapt Mathics interface to SymPy upgrade from 1.8 to 1.10 (resp.1.9)

58bf1ba Trac #33520: scipy, networkx: Update install-requires.txt

d20cc49 Trac #33139: Fix sagemath_doc_html build failure on Cygwin

e39a3a2 Trac #31924: sage -t: Do not run pytest on individual Python files unless they match the pytest file pattern

a275e1f Trac #33607: jupyterlab: Use jupyterlab-server < 2.11

949bb36 Trac #33523: Fix crosslinks in algebras catalog

e3fce64 Trac #33469: %matplotlib widget does not work in the scope of %display latex

baf6b19 Trac #33443: slow doctest improvements (isogeny_small_degree, function_field, doctest/test.py)

b11471b Trac #33393: Implement Krawtchouk, Meixner, and Hahn polynomials

a54d7ce Trac #33114: Feature.require vs. is_present, is_functional

c399fc0 Trac #32364: every quaternion lies in every quaternion order

5166ac1 Trac #20343: Add file sage/misc/latex_standalone.py and class TikzPicture

0991f82 Trac #33547: Update sympy to 1.10.1

0d0bb77 Trac #33508: OpenSSL 3.0.2 security update

56d233b Trac #33227: SystemError: calling remove_from_pari_stack() inside sig_on() in expression.pyx

43474c9 Updated SageMath version to 9.6.beta6

merge was not clean: conflicts in src/sage/schemes/hyperelliptic_curves/monsky_washnitzer.py

@fchapoton
Copy link
Contributor

comment:15

probably conflict with #33558

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 1, 2022

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

fc660bbget rid of `__nonzero__` aliases for __bool__
82dffdfconvert `__nonzero__` to __bool__
969cbcdeven less __nonzero__
b16bc4amanual fixes for __nonzero__
cf98130fine tuning : full removal of __nonzero__
b24ef83Merge branch 'u/chapoton/33558' in 9.6.B6
6f453c1Merge branch 'u/chapoton/33558' of https://github.com/sagemath/sagetrac-mirror into public/coercion/special_hyperelliptic_quotient_ring-33576

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 1, 2022

Changed commit from 7c47495 to 6f453c1

@tscrim
Copy link
Collaborator Author

tscrim commented Apr 1, 2022

comment:17

Indeed. Trivial conflict (the same ones on #33497).

@vbraun
Copy link
Member

vbraun commented Apr 3, 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

3 participants