Ticket #12476 (closed defect: fixed)

Opened 15 months ago

Last modified 15 months ago

Little fixes for a major speedup of join/meet matrices for FiniteLatticePoset

Reported by: hivert Owned by: sage-combinat
Priority: major Milestone: sage-5.0
Component: combinatorics Keywords: poset, matrix, Cernay2012
Cc: Work issues:
Report Upstream: N/A Reviewers: Florent Hivert, Nicolas M. Thiéry
Authors: Florent Hivert, Nathann Cohen Merged in: sage-5.0.beta6
Dependencies: #10998 Stopgaps:

Description (last modified by hivert) (diff)

Before 

sage: %time posets.BooleanLattice(8)
CPU times: user 10.42 s, sys: 0.01 s, total: 10.43 s
Wall time: 10.51 s
Finite lattice containing 256 elements

After 

sage: %time posets.BooleanLattice(8)
CPU times: user 0.75 s, sys: 0.01 s, total: 0.76 s
Wall time: 0.84 s
Finite lattice containing 256 elements

Apply: trac_12476-lattice_join_matrix_speedup-fh.2.patch Download

Attachments

trac_12476-lattice_join_matrix_speedup-fh.2.patch Download (5.0 KB) - added by hivert 15 months ago.

Change History

comment:1 Changed 15 months ago by ncohen

  • Status changed from new to needs_review

Helloooooooooooo !!!

I added two modifications (which did not appear to make much of a difference) but also make the code slightly easier to read. There already was a reference, I added another, no one is hurt :-)

If you are ok with this second set of modifications you can set the ticket to "positive_review".

Nathann

comment:2 Changed 15 months ago by nthiery

  • Status changed from needs_review to positive_review
  • Reviewers set to Florent Hivert, Nicolas M. Thiéry
  • Summary changed from Join matrix is unreasonably slow to Little fixes for a major speedup of join/meet matrices for FiniteLatticePoset

I folded the reviewer's patch in the original patch. All test pass and the new code is semantically equivalent, but way faster. Positive review! Thanks!

comment:3 Changed 15 months ago by nthiery

  • Dependencies set to #10988
  • Description modified (diff)

comment:4 follow-up: ↓ 5 Changed 15 months ago by hivert

  • Status changed from positive_review to needs_work

There is a little problems with duplicated references in meet_matrix and join_matrix.

Changed 15 months ago by hivert

comment:5 in reply to: ↑ 4 Changed 15 months ago by hivert

  • Status changed from needs_work to needs_review
  • Description modified (diff)

Replying to hivert:

There is a little problems with duplicated references in meet_matrix and join_matrix.

Fixed.

comment:6 Changed 15 months ago by ncohen

  • Status changed from needs_review to positive_review

Nice catch ! There's no warning anymore when I generate the doc, so I guess the new patch can go too :-)

Nathann

comment:7 follow-up: ↓ 8 Changed 15 months ago by jdemeyer

How is this related to #10988 ???

comment:8 in reply to: ↑ 7 Changed 15 months ago by nthiery

Replying to jdemeyer:

How is this related to #10988 ???

#10988 refactors a lot all the poset code; so we did not even try if this one would, by plain piece of luck, commute with it. Since #10988 is now in Sage (thanks btw!), does this have any importance?

comment:9 follow-up: ↓ 10 Changed 15 months ago by jdemeyer

  • Dependencies changed from #10988 to #10998

I guess you mean #10998 then...

comment:10 in reply to: ↑ 9 Changed 15 months ago by nthiery

Replying to jdemeyer:

I guess you mean #10998 then...

Oh, yes, sorry!!!

A dependence upon #10988 would indeed be pretty scary :-)

comment:11 Changed 15 months ago by jdemeyer

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