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

Minor fix to LiE optional SPKG #13389

Closed
kini opened this issue Aug 22, 2012 · 15 comments
Closed

Minor fix to LiE optional SPKG #13389

kini opened this issue Aug 22, 2012 · 15 comments

Comments

@kini
Copy link
Collaborator

kini commented Aug 22, 2012

This is a followup to #12983. See jhpalmieri's mail to sage-combinat-devel.

SPKG: http://wstein.org/home/keshav/files/lie-2.2.2.p5.spkg

Also, apply attachment: trac_13389-path-capitalization.patch to $SAGE_ROOT/devel/sage.

CC: @jhpalmieri

Component: packages: optional

Author: Keshav Kini

Reviewer: John Palmieri

Merged: sage-5.4.beta0

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

@kini kini added this to the sage-5.3 milestone Aug 22, 2012
@kini

This comment has been minimized.

@jhpalmieri
Copy link
Member

comment:3

Also, the use of -i with sed is not portable, and breaks installation on Solaris: the file local/bin/lie still refers to the build directory. Something like this will fix it:

diff --git a/spkg-install b/spkg-install
--- a/spkg-install
+++ b/spkg-install
@@ -22,7 +22,8 @@ make CC="$CC" || die "Error building LiE
 
 # relocating
 cd ..
-sed -i -e "s'$PWD/src'$SAGE_LOCAL/lib/LiE'" src/lie
+sed -e "s'$PWD/src'$SAGE_LOCAL/lib/LiE'" src/lie > src/lie_new
+mv src/lie_new src/lie
 rm -rf "$SAGE_LOCAL"/lib/lie # clean up old versions
 rm -rf "$SAGE_LOCAL"/bin/lie "$SAGE_LOCAL"/lib/LiE
 mv src/lie "$SAGE_LOCAL"/bin/

@kini
Copy link
Collaborator Author

kini commented Aug 22, 2012

comment:4

I see. This is now fixed too. :)

@kini
Copy link
Collaborator Author

kini commented Aug 22, 2012

diff of latest commit in SPKG, for review purposes

@jhpalmieri
Copy link
Member

comment:5

Attachment: lie.diff.gz

Sorry, I keep finding more issues. The file sage/interfaces/lie.py expects LiE to be installed in local/lib/lie, not local/lib/LiE. So the current setup leads to doctest failures.

Also, the file local/bin/lie should be executable. I guess this was a side effect of changing how sed was used.

@kini
Copy link
Collaborator Author

kini commented Aug 22, 2012

Attachment: trac_13389-path-capitalization.patch.gz

apply to $SAGE_ROOT/devel/sage

@kini
Copy link
Collaborator Author

kini commented Aug 22, 2012

comment:6

Fixed, and added a patch to the library. I chose to use the path local/lib/LiE because it seems to be less generic than local/lib/lie and less likely to cause conflicts. Such a contingency is probably not really worthy consideration, but it is only three letters long, after all...

@kini

This comment has been minimized.

@kini
Copy link
Collaborator Author

kini commented Aug 22, 2012

comment:7

Patchbot: apply trac_13389-path-capitalization.patch

@jhpalmieri
Copy link
Member

comment:8

There is still a doctest failure on some platforms:

**********************************************************************
File "/scratch/palmieri/sage-5.3.beta2/devel/sage/sage/interfaces/lie.py", line 504:
    sage: lie.trait_names() # optional - lie
Expected:
    ['Cartan_type',
     'cent_roots',
     ...
     'n_comp']
Got:
    ['history', 'version', 'operators', 'function', 'type', 'integer', 'vector', 'matrix', 'group', 'text', 'polynomial', 'special', 'help', 'assignment', 'if', 'for', 'commands', 'on', 'off', 'quit', 'edit', 'break', 'error', 'print', 'void', 'files', 'write', 'read', 'monitor', 'save', 'setdefault', 'finish', 'Cartan_type', 'cent_roots', 'centr_type', 'closure', 'fundam', 'canonical', 'dominant', 'filter_dom', 'W_action', 'W_rt_action', 'W_word', 'from_part', 'to_part', 'res_mat', 'n_rows', 'n_cols', 'vecmat', 'save_mat', 'unique', 'sort', 'center', 'diagram', 'dim', 'Lie_code', 'Lie_rank', 'Cartan', 'det_Cartan', 'high_root', 'i_Cartan', 'n_pos_roots', 'pos_roots', 'exponents', 'W_order', 'adjoint', 'max_sub', 'res_mat', 'res_mat', 'n_comp', 'Lie_group', 'orbit', 'partitions', 'Adams', 'Adams', 'alt_tensor', 'alt_tensor', 'max_sub', 'p_tensor', 'p_tensor', 'sym_tensor', 'sym_tensor', 'null', 'null', 'all_one', 'all_one', 'poly_null', 'poly_one', 'id', 'factor', 'get_mat', 'save_string', 'get_string', 'dominant', 'filter_dom', 'W_action', 'W_orbit', 'from_part', 'to_part', 'alt_dom', 'alt_dom', 'alt_W_sum', 'branch', 'collect', 'collect', 'contragr', 'decomp', 'Demazure', 'Demazure', 'dim', 'dom_char', 'dom_char', 'LR_tensor', 'spectrum', 'tensor', 'tensor', 'v_decomp', 'degree', 'expon', 'coef', 'n_vars', 'length', 'Cartan', 'cent_roots', 'centr_type', 'dom_weights', 'inprod', 'norm', 'Bruhat_desc', 'Bruhat_desc', 'Bruhat_leq', 'canonical', 'dominant', 'KL_poly', 'length', 'long_word', 'l_reduce', 'lr_reduce', 'orbit', 'reduce', 'reflection', 'R_poly', 'r_reduce', 'W_action', 'W_action', 'W_orbit', 'W_orbit_size', 'W_order', 'W_rt_action', 'W_rt_action', 'W_rt_orbit', 'W_word', 'class_ord', 'from_part', 'next_part', 'next_perm', 'next_tabl', 'next_tabl', 'print_tab', 'RS', 'RS', 'sign_part', 'shape', 'sym_char', 'sym_char', 'sym_orbit', 'tableaux', 'to_part', 'trans_part', 'alt_dom', 'alt_dom', 'alt_W_sum', 'branch', 'contragr', 'Demazure', 'Demazure', 'dim', 'dom_char', 'dom_char', 'LR_tensor', 'plethysm', 'plethysm', 'spectrum', 'tensor', 'tensor', 'v_decomp', 'size', 'matvec', 'sort']
**********************************************************************

If you want to fix it, you can (e.g., test whether 'Cartan' in lie.trait_names() returns True). It's not very important to me since it's an optional spkg, and the documentation needs a lot of work anyway: do any of the methods actually have docstrings beyond an EXAMPLES block?

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri

@kini
Copy link
Collaborator Author

kini commented Aug 22, 2012

comment:9

Yeah, I think that can be done on a different ticket...

@kini
Copy link
Collaborator Author

kini commented Aug 22, 2012

comment:10

Thanks for the review!

@jdemeyer jdemeyer modified the milestones: sage-5.3, sage-5.4 Aug 23, 2012
@haraldschilly
Copy link
Member

comment:12

the new optional spkg is uploaded to the server+mirrors

@jdemeyer
Copy link

jdemeyer commented Sep 3, 2012

Merged: sage-5.4.beta0

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

4 participants