Opened 7 years ago
Closed 5 years ago
#10670 closed defect (fixed)
Mobius matrices of posets are integer matrices
Reported by: | chapoton | Owned by: | hivert |
---|---|---|---|
Priority: | minor | Milestone: | sage-5.0 |
Component: | combinatorics | Keywords: | poset, matrix, Cernay2012 |
Cc: | chapoton, sage-combinat | Merged in: | sage-5.0.beta5 |
Authors: | Frédéric Chapoton, Florent Hivert | Reviewers: | Florent Hivert, Nicolas M. Thiéry |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #10998 | Stopgaps: |
Description (last modified by )
I have noticed the following problem.
P=Posets.PentagonPoset() P.mobius_function_matrix().parent() Full MatrixSpace of 5 by 5 sparse matrices over Rational Field
The Mobius function of a poset should really be an integer matrix. This can be achieved by using change_ring :
P.mobius_function_matrix().change_ring(ZZ).parent() Full MatrixSpace of 5 by 5 sparse matrices over Integer Ring
The patch does this by default. It also adds optional arguments to choose sparse/dense and the base ring. Same thing for lequal_matrix. It also makes sure those matrices are immutable, and improves a bit the redefinition of .mobius_function when the mobius function has been calculated.
Apply:
Attachments (1)
Change History (16)
comment:1 Changed 7 years ago by
- Milestone sage-5.0 deleted
comment:2 Changed 6 years ago by
- Description modified (diff)
comment:3 Changed 6 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:4 Changed 6 years ago by
- Cc chapoton sage-combinat added; combinatorics removed
comment:5 Changed 6 years ago by
- Description modified (diff)
comment:6 follow-up: ↓ 7 Changed 6 years ago by
- Owner changed from sage-combinat to (none)
- Reviewers set to Florent Hivert
comment:7 in reply to: ↑ 6 Changed 6 years ago by
- Status changed from needs_review to needs_work
Right now, I'm having lunch, but I'll probably find the time to post a review patch this evening if you don't beat me. If you start working on it please put a message here so that we avoid doing it twice. Of course I'll do the same. In any case one of us will have to do some review and the other to write some code.
I'm on It. Please don't touch anything !
comment:8 Changed 6 years ago by
- Dependencies set to #10998
- Description modified (diff)
- Status changed from needs_work to needs_review
I reworked the patch. Unfortunately on the way I got a dependency on #10998
Florent
comment:9 Changed 6 years ago by
Trying to help the build bot to do its job
Apply trac_10670_integral_mobius_matrix_for_posets-fh.patch
comment:10 Changed 6 years ago by
- Owner changed from (none) to hivert
comment:11 Changed 6 years ago by
- Keywords Cernay2012 added
comment:12 Changed 6 years ago by
- Description modified (diff)
- Reviewers changed from Florent Hivert to Florent Hivert, Nicolas M. Thiéry
comment:13 Changed 6 years ago by
We finalized the patch together with Florent. Positive review on the version I just uploaded, assuming the test pass (I am running them).
comment:14 Changed 6 years ago by
- Milestone set to sage-5.0
- Status changed from needs_review to positive_review
Except for a timeout in sage/sandpiles/sandpile.py, all tests passed on sage-5.0.beta2 with the following patches applied::
trac_11003-folded.patch trac_10998-categories-posets-nt.patch trac_10670_integral_mobius_matrix_for_posets-fh.patch trac_11382-subposet_to_vertex_speedup-fh.patch trac_12476-lattice_join_matrix_speedup-fh.patch trac_11118-finite_enumset_list_cache-fh.patch
Setting a positive review. Thanks Frédéric and Florent!
Changed 6 years ago by
comment:15 Changed 5 years ago by
- Merged in set to sage-5.0.beta5
- Resolution set to fixed
- Status changed from positive_review to closed
Hi Frédéric,
Thanks for taking care of this problem.
Here are a few remarks:
QQ
is a bug, then you should make sure by a doctest that the bug is indeed fixed, indicating in comment the ticket number for this bug.ring
with a default valueZZ
to avoid changing the ring twice ? In this case for consistency,.mobius_function_matrix()
should also take this optional parameter too. What do you think ?Right now, I'm having lunch, but I'll probably find the time to post a review patch this evening if you don't beat me. If you start working on it please put a message here so that we avoid doing it twice. Of course I'll do the same. In any case one of us will have to do some review and the other to write some code.
Florent