#28189 closed enhancement (fixed)
prefer "X in Fields()" rather than "X.is_field()"
Reported by:  vdelecroix  Owned by:  

Priority:  major  Milestone:  sage9.0 
Component:  algebra  Keywords:  
Cc:  Merged in:  
Authors:  Vincent Delecroix  Reviewers:  Frédéric Chapoton 
Report Upstream:  N/A  Work issues:  
Branch:  4981d44 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description (last modified by )
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 3 years ago by
 Branch set to u/vdelecroix/28189
 Commit set to 9997ea94f4975880cace4e89745596b399f6a7f1
 Description modified (diff)
 Status changed from new to needs_review
comment:2 Changed 3 years ago by
 Commit changed from 9997ea94f4975880cace4e89745596b399f6a7f1 to 2a2bbb4c5dbbf3ca18ed73c0b33358b7e82f8e5d
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2a2bbb4  28189: matrices

comment:3 Changed 3 years ago by
 Description modified (diff)
comment:4 Changed 3 years ago by
 Commit changed from 2a2bbb4c5dbbf3ca18ed73c0b33358b7e82f8e5d to 2d16f89a1802abf7181f73562ce12ec4ceebb443
comment:5 Changed 3 years ago by
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 2 years ago by
 Reviewers set to Frédéric Chapoton
 Status changed from needs_review to positive_review
ok, looks good.
comment:7 Changed 2 years ago by
Merci Frédéric!
comment:9 Changed 2 years ago by
probably with #27473
comment:10 Changed 2 years ago by
 Commit changed from 2d16f89a1802abf7181f73562ce12ec4ceebb443 to 4ef1429d7b5855cc96b739d21725da399fa72dfc
comment:11 Changed 2 years ago by
 Status changed from needs_work to needs_review
comment:12 Changed 2 years ago by
Not clear to me what conflict was resolved and how. Could you explain, please ?
comment:13 Changed 2 years ago by
 Status changed from needs_review to needs_work
Oh I see. The conflict is not merged yet.
comment:14 Changed 2 years ago by
now you can see and fix the conflict
comment:15 Changed 2 years ago by
 Branch changed from u/vdelecroix/28189 to public/ticket/28189
 Commit changed from 4ef1429d7b5855cc96b739d21725da399fa72dfc to f394978b72c84b24b6851e3a812a91bf5bdf8a58
 Status changed from needs_work to needs_review
comment:16 Changed 2 years ago by
 Commit changed from f394978b72c84b24b6851e3a812a91bf5bdf8a58 to 63ad0a3a57e807d342fbbef1efae3c398c859346
Branch pushed to git repo; I updated commit sha1. New commits:
63ad0a3  trac 28189 fix use of _Fields

comment:17 Changed 2 years ago by
And I made a mistake in the merge, now fixed.
comment:18 Changed 2 years ago by
 Status changed from needs_review to positive_review
and set to positive again.
comment:19 Changed 2 years ago by
Merci Frédéric!
comment:21 Changed 2 years ago by
and the merge is not trivial at all. Vincent, would you do it please ?
comment:22 Changed 2 years ago by
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 2 years ago by
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 2 years ago by
Can do...
comment:25 Changed 2 years ago by
Nice, thank you very much! :)
comment:26 Changed 2 years ago by
 Milestone changed from sage8.9 to sage9.0
 Status changed from needs_work to needs_review
comment:27 Changed 2 years ago by
 Branch changed from public/ticket/28189 to u/vdelecroix/28189
 Commit changed from 63ad0a3a57e807d342fbbef1efae3c398c859346 to 7767b304009dea856ebf67460ca3f7d0440da51f
comment:28 Changed 2 years ago by
New: a failing doctest in src/sage/matrix/matrix0.pyx
comment:29 followup: ↓ 30 Changed 2 years ago by
Is this a py2only 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 2 years ago by
Replying to jhpalmieri:
Is this a py2only 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 2 years ago by
 Commit changed from 7767b304009dea856ebf67460ca3f7d0440da51f to 4981d44dbb084eb91be19d9acd2662cffaed1d4f
Branch pushed to git repo; I updated commit sha1. New commits:
4981d44  fix accidental merge with #28402

comment:32 Changed 2 years ago by
now fixed
comment:34 Changed 2 years ago by
 Branch changed from u/vdelecroix/28189 to 4981d44dbb084eb91be19d9acd2662cffaed1d4f
 Resolution set to fixed
 Status changed from positive_review to closed
comment:35 Changed 2 years ago by
 Commit 4981d44dbb084eb91be19d9acd2662cffaed1d4f deleted
This has broken the inversion of matrices over ZZ, see sagedevel thread.
comment:36 Changed 2 years ago by
I think the problem is that the new matrix inverse_of_unit
is shadowing the old one from categories.rings
.
comment:37 Changed 2 years ago by
followup ticket in #28570 to try and fix the broken things ; please help
New commits:
28189: matrices TMP COMMIT