Opened 6 years ago

Closed 5 years ago

#18922 closed enhancement (fixed)

Speedup creation of spin crystals

Reported by: tscrim Owned by: sage-combinat
Priority: major Milestone: sage-6.8
Component: combinatorics Keywords: spin crystals
Cc: sage-combinat, aschilling Merged in:
Authors: Travis Scrimshaw Reviewers: Anne Schilling
Report Upstream: N/A Work issues:
Branch: d237ec2 (Commits) Commit: d237ec2e56b50959510fcab2f9f58c2d255fe271
Dependencies: Stopgaps:

Description

Currently, when we construct a spin crystal, we build the full digraph to implement a comparison operation. This ticket, in effect, makes constructing the digraph a lazy attribute which gets called when doing the comparison operation.

Change History (8)

comment:1 Changed 6 years ago by tscrim

  • Branch set to public/crystals/speedup_spin_construction-18922
  • Commit set to 7bbaef2084bf398af3e95ee79472484d1f7042b0
  • Status changed from new to needs_review

I also did some cleanup:

  • I removed the caching of the elements since the e/f operations aren't cached (which I think is where the real speed gains are) and it was a hack IMO.
  • I removed the list command because that (and the respective caching) is done by FiniteEnumeratedSets (and will result in a speedup for iteration after calling .list() once).

New commits:

​7bbaef2Speed up creation of spin crystals.
Last edited 6 years ago by tscrim (previous) (diff)

comment:2 Changed 6 years ago by aschilling

Instead of deleting the tests for the methods that you removed, it would be better if they would be moved (perhaps as tests for the class). Also, could you provide timings and an example where this was a problem before?

Thanks,

Anne

comment:3 Changed 6 years ago by tscrim

The .list() test is already in the function which creates the Bn spin crystal object, so I just added back the element construction example to the _element_constructor_ and added a test to check for consistency. Here are my timings.

With branch: 

sage: %time C = crystals.SpinsPlus(['D',6])
CPU times: user 472 µs, sys: 58 µs, total: 530 µs
Wall time: 542 µs
sage: %time C = crystals.SpinsPlus(['D',7])
CPU times: user 453 µs, sys: 56 µs, total: 509 µs
Wall time: 523 µs
sage: %time C = crystals.SpinsMinus(['D',8])
CPU times: user 0 ns, sys: 0 ns, total: 0 ns
Wall time: 956 µs
sage: %time C = crystals.Spins(['B',10])
CPU times: user 488 µs, sys: 56 µs, total: 544 µs
Wall time: 490 µs

Before: 

sage: %time C = crystals.SpinsPlus(['D',6])
CPU times: user 465 ms, sys: 69.7 ms, total: 535 ms
Wall time: 549 ms
sage: %time C = crystals.SpinsPlus(['D',7])
CPU times: user 52 ms, sys: 12.7 ms, total: 64.7 ms
Wall time: 58.6 ms
sage: %time C = crystals.SpinsMinus(['D',8])
CPU times: user 223 ms, sys: 20.6 ms, total: 244 ms
Wall time: 228 ms
sage: %time C = crystals.Spins(['B',10])
I got tired after waiting for 10 minutes and killed it and then ran this:
sage: %time C = crystals.Spins(['B',8])
CPU times: user 2.59 s, sys: 24 ms, total: 2.61 s
Wall time: 2.58 s

comment:4 Changed 6 years ago by git

  • Commit changed from 7bbaef2084bf398af3e95ee79472484d1f7042b0 to 12277f1bca950d05d7936874f99e1237422dee31

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

​12277f1Added back doctest.

comment:5 follow-up: Changed 5 years ago by git

  • Commit changed from 12277f1bca950d05d7936874f99e1237422dee31 to d237ec2e56b50959510fcab2f9f58c2d255fe271

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

​d237ec2Merge branch 'develop' into public/crystals/speedup_spin_construction-18922

comment:6 in reply to: ↑ 5 Changed 5 years ago by aschilling

Ok, looks good!

comment:7 Changed 5 years ago by aschilling

  • Reviewers set to Anne Schilling
  • Status changed from needs_review to positive_review

comment:8 Changed 5 years ago by vbraun

  • Branch changed from public/crystals/speedup_spin_construction-18922 to d237ec2e56b50959510fcab2f9f58c2d255fe271
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.