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

prefer "X in Fields()" rather than "X.is_field()" #28189

Closed
videlec opened this issue Jul 13, 2019 · 49 comments
Closed

prefer "X in Fields()" rather than "X.is_field()" #28189

videlec opened this issue Jul 13, 2019 · 49 comments

Comments

@videlec
Copy link
Contributor

videlec commented Jul 13, 2019

X.is_field() fails for many parent X. The matrix code relies a lot on the distinction ring/field. We change the code in the matrix folder to make the functions work for more base rings.

The ticket also

Component: algebra

Author: Vincent Delecroix

Branch: 4981d44

Reviewer: Frédéric Chapoton

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

@videlec videlec added this to the sage-8.9 milestone Jul 13, 2019
@videlec
Copy link
Contributor Author

videlec commented Jul 13, 2019

New commits:

9997ea928189: matrices TMP COMMIT

@videlec

This comment has been minimized.

@videlec
Copy link
Contributor Author

videlec commented Jul 13, 2019

Commit: 9997ea9

@videlec
Copy link
Contributor Author

videlec commented Jul 13, 2019

Branch: u/vdelecroix/28189

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 13, 2019

Changed commit from 9997ea9 to 2a2bbb4

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 13, 2019

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

2a2bbb428189: matrices

@videlec

This comment has been minimized.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2019

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

becd03bfix a doctest in judson-abstract-algebra
2d16f89also avoid is_integral_domain

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 17, 2019

Changed commit from 2a2bbb4 to 2d16f89

@videlec
Copy link
Contributor Author

videlec commented Jul 17, 2019

comment:5

Kwankyu: this branch introduces a serious speedup in function_field code that I hardly explain... you might want to investigate.

Git branch: matrices2                             Git branch: develop
sage -t rings/function_field/differential.py      sage -t rings/function_field/differential.py
    [220 tests, 2.46 s]                               [220 tests, 2.81 s]
sage -t rings/function_field/divisor.py           sage -t rings/function_field/divisor.py
    [200 tests, 3.42 s]                               [200 tests, 3.72 s]
sage -t rings/function_field/ideal.py             sage -t rings/function_field/ideal.py
    [915 tests, 8.90 s]                               [915 tests, 9.51 s]
sage -t rings/function_field/valuation_ring.py    sage -t rings/function_field/valuation_ring.py
    [52 tests, 0.23 s]                                [52 tests, 0.33 s]
sage -t rings/function_field/order.py             sage -t rings/function_field/order.py
    [421 tests, 1.71 s]                               [421 tests, 2.18 s]
sage -t rings/function_field/maps.py              sage -t rings/function_field/maps.py
    [378 tests, 3.77 s]                               [378 tests, 5.44 s]
sage -t rings/function_field/constructor.py       sage -t rings/function_field/constructor.py
    [42 tests, 0.08 s]                                [42 tests, 0.10 s]
sage -t rings/function_field/element.pyx          sage -t rings/function_field/element.pyx
    [275 tests, 1.27 s]                               [275 tests, 1.88 s]
sage -t rings/function_field/function_field.py    sage -t rings/function_field/function_field.py
    [743 tests, 33.02 s]                              [743 tests, 34.50 s]
sage -t rings/function_field/function_field_valua sage -t rings/function_field/function_field_valua
    [338 tests, 1.97 s]                               [338 tests, 1.97 s]
sage -t rings/function_field/place.py             sage -t rings/function_field/place.py
    [197 tests, 4.61 s]                               [197 tests, 5.24 s]

@fchapoton
Copy link
Contributor

comment:6

ok, looks good.

@fchapoton
Copy link
Contributor

Reviewer: Frédéric Chapoton

@videlec
Copy link
Contributor Author

videlec commented Aug 24, 2019

comment:7

Merci Frédéric!

@vbraun
Copy link
Member

vbraun commented Aug 25, 2019

comment:8

Merge conflict

@fchapoton
Copy link
Contributor

comment:9

probably with #27473

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2019

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

c4a4dc928189: use "in Fields()" rather than ".is_field()"
fac455d28189: doctest fix for judson-abstract-algebra
4ef142928189: use "in IntegralDomains()" rather than ".is_integral_domain()"

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 27, 2019

Changed commit from 2d16f89 to 4ef1429

@fchapoton
Copy link
Contributor

comment:12

Not clear to me what conflict was resolved and how. Could you explain, please ?

@videlec
Copy link
Contributor Author

videlec commented Aug 28, 2019

comment:13

Oh I see. The conflict is not merged yet.

@videlec
Copy link
Contributor Author

videlec commented Sep 24, 2019

comment:23

I can rebase but I will not spend my life doing it. Volker, could you merge it first in the 9.0.beta0 if I rebase?

@vbraun
Copy link
Member

vbraun commented Sep 24, 2019

comment:24

Can do...

@DeRhamSource
Copy link
Mannequin

DeRhamSource mannequin commented Sep 24, 2019

comment:25

Nice, thank you very much! :)

@videlec
Copy link
Contributor Author

videlec commented Sep 24, 2019

New commits:

5d92db328189: prefer R in Fields() rather than R.is_field()
7767b3028189: fix doctest in judson-abstract-algebra book

@videlec
Copy link
Contributor Author

videlec commented Sep 24, 2019

Changed branch from public/ticket/28189 to u/vdelecroix/28189

@videlec
Copy link
Contributor Author

videlec commented Sep 24, 2019

Changed commit from 63ad0a3 to 7767b30

@fchapoton
Copy link
Contributor

comment:28

New: a failing doctest in src/sage/matrix/matrix0.pyx

@jhpalmieri
Copy link
Member

comment:29

Is this a py2-only failure? From my limited experimentation, it looks like build_inverse_from_augmented_sparse behaves differently in Python 2 vs. 3.

@mwageringel
Copy link

comment:30

Replying to @jhpalmieri:

Is this a py2-only failure? From my limited experimentation, it looks like build_inverse_from_augmented_sparse behaves differently in Python 2 vs. 3.

Yes, see also #28402. It looks like the fix there was accidentally undone here.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 24, 2019

Changed commit from 7767b30 to 4981d44

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Sep 24, 2019

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

4981d44fix accidental merge with #28402

@videlec
Copy link
Contributor Author

videlec commented Sep 25, 2019

comment:32

now fixed

@fchapoton
Copy link
Contributor

comment:33

ok, bien

@vbraun
Copy link
Member

vbraun commented Oct 3, 2019

Changed branch from u/vdelecroix/28189 to 4981d44

@fchapoton
Copy link
Contributor

Changed commit from 4981d44 to none

@fchapoton
Copy link
Contributor

comment:35

This has broken the inversion of matrices over ZZ, see sage-devel thread.

@jhpalmieri
Copy link
Member

comment:36

I think the problem is that the new matrix inverse_of_unit is shadowing the old one from categories.rings.

@fchapoton
Copy link
Contributor

comment:37

follow-up ticket in #28570 to try and fix the broken things ; please help

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

5 participants