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
- Branch set to public/crystals/speedup_spin_construction-18922
- Commit set to 7bbaef2084bf398af3e95ee79472484d1f7042b0
- Status changed from new to needs_review
comment:2 Changed 6 years ago by
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
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
- Commit changed from 7bbaef2084bf398af3e95ee79472484d1f7042b0 to 12277f1bca950d05d7936874f99e1237422dee31
Branch pushed to git repo; I updated commit sha1. New commits:
12277f1 | Added back doctest.
|
comment:5 follow-up: ↓ 6 Changed 5 years ago by
- Commit changed from 12277f1bca950d05d7936874f99e1237422dee31 to d237ec2e56b50959510fcab2f9f58c2d255fe271
Branch pushed to git repo; I updated commit sha1. New commits:
d237ec2 | Merge branch 'develop' into public/crystals/speedup_spin_construction-18922
|
comment:6 in reply to: ↑ 5 Changed 5 years ago by
Ok, looks good!
comment:7 Changed 5 years ago by
- Reviewers set to Anne Schilling
- Status changed from needs_review to positive_review
comment:8 Changed 5 years ago by
- Branch changed from public/crystals/speedup_spin_construction-18922 to d237ec2e56b50959510fcab2f9f58c2d255fe271
- Resolution set to fixed
- Status changed from positive_review to closed
I also did some cleanup:
list
command because that (and the respective caching) is done byFiniteEnumeratedSets
(and will result in a speedup for iteration after calling.list()
once).New commits:
Speed up creation of spin crystals.