Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speedup creation of spin crystals #18922

Closed
tscrim opened this issue Jul 19, 2015 · 12 comments
Closed

Speedup creation of spin crystals #18922

tscrim opened this issue Jul 19, 2015 · 12 comments

Comments

@tscrim
Copy link
Collaborator

tscrim commented Jul 19, 2015

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.

CC: @sagetrac-sage-combinat @anneschilling

Component: combinatorics

Keywords: spin crystals

Author: Travis Scrimshaw

Branch/Commit: d237ec2

Reviewer: Anne Schilling

Issue created by migration from https://trac.sagemath.org/ticket/18922

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 19, 2015

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 19, 2015

comment:1

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.

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 19, 2015

Commit: 7bbaef2

@anneschilling
Copy link

comment:2

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

@tscrim
Copy link
Collaborator Author

tscrim commented Jul 21, 2015

comment:3

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2015

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

12277f1Added back doctest.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jul 21, 2015

Changed commit from 7bbaef2 to 12277f1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2015

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

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

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 5, 2015

Changed commit from 12277f1 to d237ec2

@anneschilling
Copy link

comment:6

Ok, looks good!

@anneschilling
Copy link

Reviewer: Anne Schilling

@vbraun
Copy link
Member

vbraun commented Aug 7, 2015

Changed branch from public/crystals/speedup_spin_construction-18922 to d237ec2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants