search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.4k stars 474 forks source link

upgrade lrcalc to 2.1 #31355

Closed edd8e884-f507-429a-b577-5d554626c0fe closed 2 years ago

edd8e884-f507-429a-b577-5d554626c0fe commented 3 years ago

lrcalc 2.1 is out https://bitbucket.org/asbuch/lrcalc/src/master/ChangeLog

Instead of rewriting Sage's Python bindings to support the new API, we replace them with a wrapper around the new upstream provided bindings

Upstream:

lrcalclib-2.1.tar.gz (lrcalc): https://sites.math.rutgers.edu/~asbuch/lrcalc/lrcalc-2.1.tar.gz lrcalc-2.1.tar.gz (lrcalc_python): https://pypi.io/packages/source/l/lrcalc/lrcalc-2.1.tar.gz

CC: @antonio-rojas @anneschilling @fchapoton @tscrim @thierry-FreeBSD @orlitzky @mkoeppe @kiwifb @asbuch @slel

Component: packages: optional

Keywords: upgrade, lrcalc

Author: Antonio Rojas, Matthias Koeppe

Branch/Commit: de4fe09

Reviewer: Matthias Koeppe, Travis Scrimshaw

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

mkoeppe commented 3 years ago

Changed author from Antonio Rojas to Antonio Rojas, Matthias Koeppe

mkoeppe commented 3 years ago

Changed reviewer from Matthias Koeppe to Matthias Koeppe, ...

tscrim commented 3 years ago
comment:89

Some comments:

Shouldn't the doc for def _lrcalc_dict_to_sage(result): go after the implementation?

I think the extra dict call here is unnecessary:

-return dict({_Partitions(i):Integer(k) for i,k in result.items()})
+return {_Partitions(la): Integer(k) for la,k in result.items()}

(I changed the variable to make it more clear that it is a partition being returned.)

-return dict({tuple(_Partitions(j) for j in i):Integer(k) for i,k in result.items()})
+return {tuple(_Partitions(mu) for mu in la): Integer(k) for la,k in result.items()}

Also, is the list call necessary here?

-return dict({Permutation(list(i)):Integer(k) for i,k in result.items()})
+return {Permutation(list(la)): Integer(k) for la,k in result.items()}

For speed and memory usage (1 object instead of 2):

     for i,k in result.items():
-        output[_Partitions(i[0])] = output.get(_Partitions(i[0]), P.zero()) + k*quantum**(i[1])
+        la = _Partitions(i[0])
+        output[la] = output.get(la, P.zero()) + k*quantum**(i[1])

I believe this could be simplified:

-        while True:
-            try:
-                word = Word([i+1 for i in next(iterator)])
-                yield SkewTableaux().from_shape_and_word(shape, word)
-            except StopIteration:
-                break
+        ST = SkewTableaux()
+        for data in iterator:
+            yield ST.from_shape_and_word(shape, Word([i+1 for i in data]))

and similarly if weight is given. However, I think it would make sense to make wt = tuple(weight) and then get the weight of the word (perhaps before even constructing Word) first to avoid creating transient objects. We also don't need word to be a subclass of Word; a list is sufficient.

antonio-rojas commented 3 years ago

Changed branch from u/mkoeppe/upgrade_lrcalc_to_2_0 to u/arojas/upgrade_lrcalc_to_2_0

antonio-rojas commented 3 years ago

Changed commit from 7a0bf0a to 2cb4511

antonio-rojas commented 3 years ago
comment:91

Replying to @tscrim:

Some comments:

Thanks for the suggestions, all implemented now.

Also, is the list call necessary here?

-return dict({Permutation(list(i)):Integer(k) for i,k in result.items()})
+return {Permutation(list(la)): Integer(k) for la,k in result.items()}

Yes, otherwise the Permutation constructor will interpret the tuple input as cycle notation.

New commits:

2cb4511Implement tscrim's suggestions, fix test output
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from 2cb4511 to e4f4e1b

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

e4f4e1bSimplify
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

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

f24ff71Merge remote-tracking branch 'origin/develop' into t/31355/upgrade_lrcalc_to_2_0
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 3 years ago

Changed commit from e4f4e1b to f24ff71

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

42ecd9aMerge remote-tracking branch 'origin' into t/31355/upgrade_lrcalc_to_2_0
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from f24ff71 to 42ecd9a

antonio-rojas commented 2 years ago
comment:95

Ping. Anything else needed here?

dimpase commented 2 years ago
comment:96

What is the minimum lrcalc version required by lrcalc_python?

antonio-rojas commented 2 years ago
comment:97

lrcalc and lrcalc_python come from the same repo, so I suppose the versions need to match.

dimpase commented 2 years ago
comment:98

A number of distrbutions have system-wide lrcalc installations, some still version 1.*, and those can be picked up by Sage.

If these are not compatible they should not be used.

antonio-rojas commented 2 years ago
comment:99

1.* will not be picked up, the ivector.h header was introduced in 2.0. Don't know about 2.0, according to repology only gentoo is shipping that. @kiwifb, any reason it hasn't been updated to 2.1?

kiwifb commented 2 years ago
comment:100

I just have been quite busy. And since @orlitzky has migrated a lot of packages to the main tree, I feel less personal pressure to update. We have 2.0 in the main tree but not 2.1 yet. I will have to see what needs to be done.

kiwifb commented 2 years ago
comment:101

lrcalc_python/SPKG.rst claims it is GPL2+ but https://bitbucket.org/asbuch/lrcalc/src/master/python/ and https://bitbucket.org/asbuch/lrcalc/src/master/python/setup.py make it pretty clear that it is GPL3.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

03e0c8fFix spkg-configure to use C and only pass for version 2.1
b944affFix license
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 42ecd9a to b944aff

antonio-rojas commented 2 years ago
comment:103

Replying to @dimpase:

A number of distrbutions have system-wide lrcalc installations, some still version 1.*, and those can be picked up by Sage.

If these are not compatible they should not be used.

Actually spkg-configure didn't work at all because it was using C++ instead of C. Fixed it now and added a check for a header only present in 2.1 so it will fail with older versions.

Fixed the license too, should be ready to go now.

kiwifb commented 2 years ago
comment:104

I have opened a PR for getting 2.1 in Gentoo and I have an ebuild ready for the python bindings for the sage-on-gentoo overlay just waiting for the PR inclusion before going live [because of dependencies requirements, cannot go in until lrcalc-2.1 is present].

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from b944aff to 5d443b9

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

5d443b9Merge remote-tracking branch 'origin/develop' into t/31355/upgrade_lrcalc_to_2_0
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

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

ffb08d8Merge remote-tracking branch 'origin' into t/31355/upgrade_lrcalc_to_2_0
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 2 years ago

Changed commit from 5d443b9 to ffb08d8

antonio-rojas commented 2 years ago
comment:108

Ping, hope to get this in 9.6

tscrim commented 2 years ago

Changed branch from u/arojas/upgrade_lrcalc_to_2_0 to u/tscrim/upgrade_lrcalc_2_1-31355

tscrim commented 2 years ago

Description changed:

--- 
+++ 
@@ -1,3 +1,8 @@
 lrcalc 2.1 is out https://bitbucket.org/asbuch/lrcalc/src/master/ChangeLog

 Instead of rewriting Sage's Python bindings to support the new API, we replace them with a wrapper around the new upstream provided bindings
+
+Upstream:
+
+lrcalclib-2.1.tar.gz (lrcalc): https://sites.math.rutgers.edu/~asbuch/lrcalc/lrcalc-2.1.tar.gz
+lrcalc-2.1.tar.gz (lrcalc_python): https://pypi.io/packages/source/l/lrcalc/lrcalc-2.1.tar.gz
tscrim commented 2 years ago

Changed commit from ffb08d8 to de4fe09

tscrim commented 2 years ago
comment:109

Does Sage's auto-download mechanism automatically rename the tarballs to their appropriate values. I am worried that both tarballs (lib and python) are called lrcalc-2.1.tar.gz on their respective upstream locations.

I made a few other little tweaks and checked the tests. If my changes are good, then it is a positive review.

New commits:

e8d0b5bMerge branch 'u/arojas/upgrade_lrcalc_to_2_0' of git://trac.sagemath.org/sage into u/arojas/upgrade_lrcalc_to_2_0
de4fe09Some improvements to lrcalc.py.
tscrim commented 2 years ago

Changed reviewer from Matthias Koeppe, ... to Matthias Koeppe, Travis Scrimshaw

mkoeppe commented 2 years ago
comment:110

Replying to @tscrim:

Does Sage's auto-download mechanism automatically rename the tarballs to their appropriate values. I am worried that both tarballs (lib and python) are called lrcalc-2.1.tar.gz on their respective upstream locations.

Yes, it is renamed to the value given in the tarball field of checksums.ini. See comment:80.

antonio-rojas commented 2 years ago
comment:111

Thanks

vbraun commented 2 years ago

Changed branch from u/tscrim/upgrade_lrcalc_2_1-31355 to de4fe09