Opened 12 months ago

Closed 9 months ago

Last modified 9 months ago

#28189 closed enhancement (fixed)

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

Reported by: vdelecroix Owned by:
Priority: major Milestone: sage-9.0
Component: algebra Keywords:
Cc: Merged in:
Authors: Vincent Delecroix Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 4981d44 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by vdelecroix)

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

  • simplifies the code of adjugate
  • handles more matrix inversion (as reported in #27869)

Change History (37)

comment:1 Changed 12 months ago by vdelecroix

  • Branch set to u/vdelecroix/28189
  • Commit set to 9997ea94f4975880cace4e89745596b399f6a7f1
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

9997ea928189: matrices TMP COMMIT

comment:2 Changed 12 months ago by git

  • Commit changed from 9997ea94f4975880cace4e89745596b399f6a7f1 to 2a2bbb4c5dbbf3ca18ed73c0b33358b7e82f8e5d

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

2a2bbb428189: matrices

comment:3 Changed 12 months ago by vdelecroix

  • Description modified (diff)

comment:4 Changed 12 months ago by git

  • Commit changed from 2a2bbb4c5dbbf3ca18ed73c0b33358b7e82f8e5d to 2d16f89a1802abf7181f73562ce12ec4ceebb443

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

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

comment:5 Changed 12 months ago by vdelecroix

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]

comment:6 Changed 10 months ago by chapoton

  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

ok, looks good.

comment:7 Changed 10 months ago by vdelecroix

Merci Frédéric!

comment:8 Changed 10 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:9 Changed 10 months ago by chapoton

probably with #27473

comment:10 Changed 10 months ago by git

  • Commit changed from 2d16f89a1802abf7181f73562ce12ec4ceebb443 to 4ef1429d7b5855cc96b739d21725da399fa72dfc

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

comment:11 Changed 10 months ago by vdelecroix

  • Status changed from needs_work to needs_review

comment:12 Changed 10 months ago by chapoton

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

comment:13 Changed 10 months ago by vdelecroix

  • Status changed from needs_review to needs_work

Oh I see. The conflict is not merged yet.

comment:14 Changed 10 months ago by chapoton

now you can see and fix the conflict

comment:15 Changed 10 months ago by chapoton

  • Branch changed from u/vdelecroix/28189 to public/ticket/28189
  • Commit changed from 4ef1429d7b5855cc96b739d21725da399fa72dfc to f394978b72c84b24b6851e3a812a91bf5bdf8a58
  • Status changed from needs_work to needs_review

I have merged.


New commits:

f394978Merge branch 'u/vdelecroix/28189' in 8.9.b9

comment:16 Changed 10 months ago by git

  • Commit changed from f394978b72c84b24b6851e3a812a91bf5bdf8a58 to 63ad0a3a57e807d342fbbef1efae3c398c859346

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

63ad0a3trac 28189 fix use of _Fields

comment:17 Changed 10 months ago by chapoton

And I made a mistake in the merge, now fixed.

comment:18 Changed 10 months ago by chapoton

  • Status changed from needs_review to positive_review

and set to positive again.

comment:19 Changed 10 months ago by vdelecroix

Merci Frédéric!

comment:20 Changed 10 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:21 Changed 10 months ago by chapoton

and the merge is not trivial at all. Vincent, would you do it please ?

comment:22 Changed 9 months ago by gh-DeRhamSource

Is a merge into 8.9 planned? I really need this ticket for vector bundles and therefore my master thesis. A merge is highly appreciated!

comment:23 Changed 9 months ago by vdelecroix

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?

comment:24 Changed 9 months ago by vbraun

Can do...

comment:25 Changed 9 months ago by gh-DeRhamSource

Nice, thank you very much! :)

comment:26 Changed 9 months ago by vdelecroix

  • Milestone changed from sage-8.9 to sage-9.0
  • Status changed from needs_work to needs_review

comment:27 Changed 9 months ago by vdelecroix

  • Branch changed from public/ticket/28189 to u/vdelecroix/28189
  • Commit changed from 63ad0a3a57e807d342fbbef1efae3c398c859346 to 7767b304009dea856ebf67460ca3f7d0440da51f

New commits:

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

comment:28 Changed 9 months ago by chapoton

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

comment:29 follow-up: Changed 9 months ago by 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.

comment:30 in reply to: ↑ 29 Changed 9 months ago by gh-mwageringel

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.

comment:31 Changed 9 months ago by git

  • Commit changed from 7767b304009dea856ebf67460ca3f7d0440da51f to 4981d44dbb084eb91be19d9acd2662cffaed1d4f

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

4981d44fix accidental merge with #28402

comment:32 Changed 9 months ago by vdelecroix

now fixed

comment:33 Changed 9 months ago by chapoton

  • Status changed from needs_review to positive_review

ok, bien

comment:34 Changed 9 months ago by vbraun

  • Branch changed from u/vdelecroix/28189 to 4981d44dbb084eb91be19d9acd2662cffaed1d4f
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:35 Changed 9 months ago by chapoton

  • Commit 4981d44dbb084eb91be19d9acd2662cffaed1d4f deleted

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

comment:36 Changed 9 months ago by jhpalmieri

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

comment:37 Changed 9 months ago by chapoton

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

Note: See TracTickets for help on using tickets.