Opened 2 years ago

Closed 2 years ago

#22642 closed enhancement (fixed)

Cythonize tensor product of crystals elements

Reported by: tscrim Owned by:
Priority: major Milestone: sage-8.0
Component: combinatorics Keywords: crystals
Cc: sage-combinat, bsalisbury1, aschilling, alubovsky Merged in:
Authors: Travis Scrimshaw Reviewers: Ben Salisbury
Report Upstream: N/A Work issues:
Branch: 1b8c800 (Commits) Commit: 1b8c800d20c16ddcceb90d98ecb725bc079cca81
Dependencies: #22429 Stopgaps:

Description

It is good to Cythonize things that can be called quite frequently.

I also go through and change the base class to ClonableArray from CombinatorialElement. This helps out things like the R-matrix computation, which relies heavily on hashing and is slow because CombinatorialElement uses the string representations to hash. However, all crystal elements should be hashable, so we can just hash the input.

I'm also taking this opportunity to do a little bit of extra cleanup of the methods.

Change History (27)

comment:1 Changed 2 years ago by tscrim

  • Branch set to public/crystals/cythonize_tensor_product_elements-22642
  • Status changed from new to needs_review

With this branch, I get:

sage: T = crystals.Tableaux(['D',5], shape=[4,2,1])
sage: %time for x in T: x
CPU times: user 4.98 s, sys: 68 ms, total: 5.05 s
Wall time: 4.88 s
sage: La = RootSystem(['D',5,1]).weight_space().fundamental_weights()
sage: B = crystals.ProjectedLevelZeroLSPaths(La[3])
sage: %time L = [hash(x) for x in tensor([B,B])]
CPU times: user 11.2 s, sys: 52 ms, total: 11.2 s
Wall time: 11.1 s

vs the old

sage: T = crystals.Tableaux(['D',5], shape=[4,2,1])
sage: %time for x in T: x
CPU times: user 8.06 s, sys: 32 ms, total: 8.09 s
Wall time: 8.03 s
sage: La = RootSystem(['D',5,1]).weight_space().fundamental_weights()
sage: B = crystals.ProjectedLevelZeroLSPaths(La[3])
sage: %time L = [hash(x) for x in tensor([B,B])]
CPU times: user 12.5 s, sys: 52 ms, total: 12.6 s
Wall time: 12.5 s

This ticket might seem like a lot, but it is really fundamentally a move to a Cython file and doing some simple optimizations.

comment:2 Changed 2 years ago by git

  • Commit set to 2e171a4682256be81bc6a0513ad62abc39bde9ac

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

2e171a4Cythonizing tensor product of crystals element.

comment:3 Changed 2 years ago by chapoton

  • Status changed from needs_review to needs_work

many failing doctests

comment:4 Changed 2 years ago by git

  • Commit changed from 2e171a4682256be81bc6a0513ad62abc39bde9ac to 7b5155386073f973e52134ced95a028eb72ce0fb

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

a6e233dMerge branch 'public/crystals/cythonize_tensor_product_elements-22642' of git://trac.sagemath.org/sage into public/crystals/cythonize_tensor_product_elements-22642
6229d3fFixing doctests by simplifying hash.
7b51553Fixing unpickling old pickles and removing readded TestParent.

comment:5 follow-up: Changed 2 years ago by tscrim

  • Status changed from needs_work to needs_review

This should take care of the failing doctests. If there are others, then its is a trivial sorting order thing, which is a little bit of a harder problem because of the poset nature of crystals.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 2 years ago by aschilling

Some trivial comments:

  • It should be "Tensor Products of Crystal Elements" not "Tensor Products of Crystals Elements"
  • Your conventions for the descriptions are not always consistent. Sometimes you use upper case letter "- i -- An element of the index set" but sometimes you also use lower case letter "- i -- an element of the index set"

comment:7 Changed 2 years ago by git

  • Commit changed from 7b5155386073f973e52134ced95a028eb72ce0fb to cfc5218e125bee556f56e5b5dd83c14b4e77268d

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

cfc5218Some small tweaks from Anne's comments.

comment:8 in reply to: ↑ 6 Changed 2 years ago by tscrim

Replying to aschilling:

  • It should be "Tensor Products of Crystal Elements" not "Tensor Products of Crystals Elements"

Fixed.

  • Your conventions for the descriptions are not always consistent. Sometimes you use upper case letter "- i -- An element of the index set" but sometimes you also use lower case letter "- i -- an element of the index set"

This was from the existing code and the move over. However, this is a good chance to move this to Sage's convention of lower case. Fixed.

comment:9 Changed 2 years ago by chapoton

still some failing doctests

comment:10 Changed 2 years ago by git

  • Commit changed from cfc5218e125bee556f56e5b5dd83c14b4e77268d to 54fbc405725523afe4842778462bfa7e08c712c0

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

54fbc40Hopefully taking care of last doctest failures.

comment:11 Changed 2 years ago by tscrim

This should take care of those doctests. Some were trivial, but some exposed that element comparison was not working correctly. I made the element classes trivial Python subclasses, which makes the sorting work correctly. I don't know why, but it doesn't result in a change in the timings and is a good workaround for now. I would say this is a bug, but it is somewhere higher up. I don't know a MWE or where to look, but IMO it shouldn't block this ticket.

comment:12 follow-up: Changed 2 years ago by git

  • Commit changed from 54fbc405725523afe4842778462bfa7e08c712c0 to c314fef1f6af261f9781b1ef23c8942b1ab86aad

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

c314fefFixing nondeterministic doctest ordering, last time.

comment:13 in reply to: ↑ 12 Changed 2 years ago by aschilling

With this branch Sage does not start after compilation on my computer.

...
[sagelib-8.0.beta2] real	0m4.607s
[sagelib-8.0.beta2] user	0m3.264s
[sagelib-8.0.beta2] sys	0m0.939s
"/Applications/sage/local/bin/sage-starts"

Testing that Sage starts...
[2017-04-28 16:17:10] SageMath version 8.0.beta2, Release Date: 2017-04-12
************************************************************************
It seems that you are attempting to run Sage from an unpacked source
tarball, but you have not compiled it yet (or maybe the build has not
finished). You should run `make` in the Sage root directory first.
If you did not intend to build Sage from source, you should download
a binary tarball instead. Read README.txt for more information.
************************************************************************
Sage failed to start up.
Please email sage-devel (http://groups.google.com/group/sage-devel)
explaining the problem and send the log file
  /Applications/sage/logs/start.log
Describe your computer, operating system, etc.
make[2]: *** [/Applications/sage/local/etc/sage-started.txt] Error 1
make[1]: *** [all] Error 2

real	15m3.416s
user	44m8.369s
sys	2m23.495s
***************************************************************
Error building Sage.

The following package(s) may have failed to build (not necessarily
during this run of 'make all'):

The build directory may contain configuration files and other potentially
helpful information. WARNING: if you now run 'make' again, the build
directory will, by default, be deleted. Set the environment variable
SAGE_KEEP_BUILT_SPKGS to 'yes' to prevent this.

make: *** [all] Error 1

comment:14 follow-up: Changed 2 years ago by tscrim

It looks like you upgraded to beta2 (via this branch) without running make build. I've never had any problems with building this branch (nor do the patchbots that work). Does Sage start for you on develop?

comment:15 in reply to: ↑ 14 Changed 2 years ago by aschilling

Replying to tscrim:

It looks like you upgraded to beta2 (via this branch) without running make build. I've never had any problems with building this branch (nor do the patchbots that work). Does Sage start for you on develop?

Yes, Sage runs for me on develop. I now remade the branch after make distclean and it is now running!

comment:16 Changed 2 years ago by git

  • Commit changed from c314fef1f6af261f9781b1ef23c8942b1ab86aad to 01ef9879c85a7d04c82f8cabf49b06fa662748f0

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

3457928Merge branch 'develop' into public/crystals/cythonize_tensor_product_elements-22642
76cbd00Merge branch 'develop' into public/crystals/cythonize_tensor_product_elements-22642
01ef987Forcing things to be well-ordered.

comment:17 Changed 2 years ago by tscrim

I made one small tweak to force that the output ordering of those dict's in kirillov_reshetikhin.py are ordered. After working on this ticket, I'm somewhat shocked those tests were passing beforehand. Anyways...now they should always be in the same order on every system.

comment:18 Changed 2 years ago by git

  • Commit changed from 01ef9879c85a7d04c82f8cabf49b06fa662748f0 to a3f391a8b405668ffa87b7513da06d1a26d83c17

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

f2edd24Fix energy function for A2 even dual.
3071be8Merge branch 'develop' into public/crystals/fix_energy_A2_even-22882
143f95dCleaning some stuff up with some crystals.
d8b50b3Fixed typo direclty -> directly.
6725c0fMerge branch 'develop' into public/crystals/improve_crystals-22429
656bcd7Merge branch 'public/sets/disjoint_union_facade-22382' of git://trac.sagemath.org/sage into public/crystals/improve_crystals-22429
c1836fcFixing some last tidbits.
cbc0ad3Merge public/crystals/fix_energy_A2_even-22882.
a3f391aMerge branch 'public/crystals/improve_crystals-22429' into public/crystals/cythonize_tensor_product_elements-22642

comment:19 Changed 2 years ago by tscrim

  • Dependencies set to #22429

Rebased over #22429.

comment:20 follow-up: Changed 2 years ago by bsalisbury1

  • Reviewers set to Ben Salisbury
  • Status changed from needs_review to positive_review

comment:21 Changed 2 years ago by tscrim

Thank you very much!!!

comment:22 in reply to: ↑ 20 Changed 2 years ago by aschilling

Hi Ben, did you actually check that none of the tests in tensor product got lost in the process of moving the file? I just want to make sure ... Anne

comment:23 Changed 2 years ago by bsalisbury1

Hi Anne,

I did... By my check, there is nothing missing that should be there. Thanks for keeping me honest!

Ben

comment:24 Changed 2 years ago by vbraun

  • Status changed from positive_review to needs_work

PDF docs don't build

comment:25 Changed 2 years ago by git

  • Commit changed from a3f391a8b405668ffa87b7513da06d1a26d83c17 to 1b8c800d20c16ddcceb90d98ecb725bc079cca81

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

3dc06b7Fixing latex typo.
1b8c800Merge branch 'public/crystals/improve_crystals-22429' into public/crystals/cythonize_tensor_product_elements-22642

comment:26 Changed 2 years ago by tscrim

  • Status changed from needs_work to positive_review

Fixed in #22429, which introduced the typo.

comment:27 Changed 2 years ago by vbraun

  • Branch changed from public/crystals/cythonize_tensor_product_elements-22642 to 1b8c800d20c16ddcceb90d98ecb725bc079cca81
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.