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:  sage8.0 
Component:  combinatorics  Keywords:  crystals 
Cc:  sagecombinat, 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 Rmatrix 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
 Branch set to public/crystals/cythonize_tensor_product_elements22642
 Status changed from new to needs_review
comment:2 Changed 2 years ago by
 Commit set to 2e171a4682256be81bc6a0513ad62abc39bde9ac
Branch pushed to git repo; I updated commit sha1. New commits:
2e171a4  Cythonizing tensor product of crystals element.

comment:3 Changed 2 years ago by
 Status changed from needs_review to needs_work
many failing doctests
comment:4 Changed 2 years ago by
 Commit changed from 2e171a4682256be81bc6a0513ad62abc39bde9ac to 7b5155386073f973e52134ced95a028eb72ce0fb
Branch pushed to git repo; I updated commit sha1. New commits:
a6e233d  Merge branch 'public/crystals/cythonize_tensor_product_elements22642' of git://trac.sagemath.org/sage into public/crystals/cythonize_tensor_product_elements22642

6229d3f  Fixing doctests by simplifying hash.

7b51553  Fixing unpickling old pickles and removing readded TestParent.

comment:5 followup: ↓ 6 Changed 2 years ago by
 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 ; followup: ↓ 8 Changed 2 years ago by
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
 Commit changed from 7b5155386073f973e52134ced95a028eb72ce0fb to cfc5218e125bee556f56e5b5dd83c14b4e77268d
Branch pushed to git repo; I updated commit sha1. New commits:
cfc5218  Some small tweaks from Anne's comments.

comment:8 in reply to: ↑ 6 Changed 2 years ago by
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
still some failing doctests
comment:10 Changed 2 years ago by
 Commit changed from cfc5218e125bee556f56e5b5dd83c14b4e77268d to 54fbc405725523afe4842778462bfa7e08c712c0
Branch pushed to git repo; I updated commit sha1. New commits:
54fbc40  Hopefully taking care of last doctest failures.

comment:11 Changed 2 years ago by
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 followup: ↓ 13 Changed 2 years ago by
 Commit changed from 54fbc405725523afe4842778462bfa7e08c712c0 to c314fef1f6af261f9781b1ef23c8942b1ab86aad
Branch pushed to git repo; I updated commit sha1. New commits:
c314fef  Fixing nondeterministic doctest ordering, last time.

comment:13 in reply to: ↑ 12 Changed 2 years ago by
With this branch Sage does not start after compilation on my computer.
... [sagelib8.0.beta2] real 0m4.607s [sagelib8.0.beta2] user 0m3.264s [sagelib8.0.beta2] sys 0m0.939s "/Applications/sage/local/bin/sagestarts" Testing that Sage starts... [20170428 16:17:10] SageMath version 8.0.beta2, Release Date: 20170412 ************************************************************************ 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 sagedevel (http://groups.google.com/group/sagedevel) 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/sagestarted.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 followup: ↓ 15 Changed 2 years ago by
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
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 ondevelop
?
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
 Commit changed from c314fef1f6af261f9781b1ef23c8942b1ab86aad to 01ef9879c85a7d04c82f8cabf49b06fa662748f0
comment:17 Changed 2 years ago by
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
 Commit changed from 01ef9879c85a7d04c82f8cabf49b06fa662748f0 to a3f391a8b405668ffa87b7513da06d1a26d83c17
Branch pushed to git repo; I updated commit sha1. New commits:
f2edd24  Fix energy function for A2 even dual.

3071be8  Merge branch 'develop' into public/crystals/fix_energy_A2_even22882

143f95d  Cleaning some stuff up with some crystals.

d8b50b3  Fixed typo direclty > directly.

6725c0f  Merge branch 'develop' into public/crystals/improve_crystals22429

656bcd7  Merge branch 'public/sets/disjoint_union_facade22382' of git://trac.sagemath.org/sage into public/crystals/improve_crystals22429

c1836fc  Fixing some last tidbits.

cbc0ad3  Merge public/crystals/fix_energy_A2_even22882.

a3f391a  Merge branch 'public/crystals/improve_crystals22429' into public/crystals/cythonize_tensor_product_elements22642

comment:20 followup: ↓ 22 Changed 2 years ago by
 Reviewers set to Ben Salisbury
 Status changed from needs_review to positive_review
comment:21 Changed 2 years ago by
Thank you very much!!!
comment:22 in reply to: ↑ 20 Changed 2 years ago by
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
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
 Status changed from positive_review to needs_work
PDF docs don't build
comment:25 Changed 2 years ago by
 Commit changed from a3f391a8b405668ffa87b7513da06d1a26d83c17 to 1b8c800d20c16ddcceb90d98ecb725bc079cca81
comment:26 Changed 2 years ago by
 Status changed from needs_work to positive_review
Fixed in #22429, which introduced the typo.
comment:27 Changed 2 years ago by
 Branch changed from public/crystals/cythonize_tensor_product_elements22642 to 1b8c800d20c16ddcceb90d98ecb725bc079cca81
 Resolution set to fixed
 Status changed from positive_review to closed
With this branch, I get:
vs the old
This ticket might seem like a lot, but it is really fundamentally a move to a Cython file and doing some simple optimizations.