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 nthiery)

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)

trac_10670_integral_mobius_matrix_for_posets-fh.patch (11.2 KB) - added by nthiery 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 7 years ago by chapoton

  • Milestone sage-5.0 deleted

comment:2 Changed 6 years ago by chapoton

  • Description modified (diff)

comment:3 Changed 6 years ago by chapoton

  • Authors set to chapoton
  • Description modified (diff)
  • Status changed from new to needs_review

comment:4 Changed 6 years ago by chapoton

  • Cc chapoton sage-combinat added; combinatorics removed

comment:5 Changed 6 years ago by chapoton

  • Description modified (diff)

comment:6 follow-up: Changed 6 years ago by hivert

  • Authors changed from chapoton to Frédéric Chapoton
  • Owner changed from sage-combinat to (none)
  • Reviewers set to Florent Hivert

Hi Frédéric,

Thanks for taking care of this problem.

Here are a few remarks:

  • The field Author below is used to give credits after each release. You should put your real name here and not your login.
  • If you think returning a matrix over 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.
  • I can easily imagine use cases where the Moebius function is needed over a different ring (eg: Z2 or a polynomial field). What about having an optional argument ring with a default value ZZ 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

comment:7 in reply to: ↑ 6 Changed 6 years ago by hivert

  • 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 hivert

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, Florent Hivert
  • 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 chapoton

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 hivert

  • Owner changed from (none) to hivert

comment:11 Changed 6 years ago by hivert

  • Keywords Cernay2012 added

comment:12 Changed 6 years ago by nthiery

  • Description modified (diff)
  • Reviewers changed from Florent Hivert to Florent Hivert, Nicolas M. Thiéry

comment:13 Changed 6 years ago by nthiery

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 nthiery

  • 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!

comment:15 Changed 5 years ago by jdemeyer

  • Merged in set to sage-5.0.beta5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.