Opened 9 years ago

Closed 9 years ago

#13389 closed enhancement (fixed)

Minor fix to LiE optional SPKG

Reported by: kini Owned by: tbd
Priority: major Milestone: sage-5.4
Component: packages: optional Keywords:
Cc: jhpalmieri Merged in: sage-5.4.beta0
Authors: Keshav Kini Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Attachments (2)

lie.diff (806 bytes) - added by kini 9 years ago.
diff of latest commit in SPKG, for review purposes
trac_13389-path-capitalization.patch (871 bytes) - added by kini 9 years ago.
apply to $SAGE_ROOT/devel/sage

Download all attachments as: .zip

Change History (15)

comment:1 Changed 9 years ago by kini

  • Status changed from new to needs_review

comment:2 Changed 9 years ago by kini

  • Description modified (diff)

comment:3 Changed 9 years ago by jhpalmieri

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:

  • spkg-install

    diff --git a/spkg-install b/spkg-install
    a b make CC="$CC" || die "Error building LiE 
    2222
    2323# relocating
    2424cd ..
    25 sed -i -e "s'$PWD/src'$SAGE_LOCAL/lib/LiE'" src/lie
     25sed -e "s'$PWD/src'$SAGE_LOCAL/lib/LiE'" src/lie > src/lie_new
     26mv src/lie_new src/lie
    2627rm -rf "$SAGE_LOCAL"/lib/lie # clean up old versions
    2728rm -rf "$SAGE_LOCAL"/bin/lie "$SAGE_LOCAL"/lib/LiE
    2829mv src/lie "$SAGE_LOCAL"/bin/

comment:4 Changed 9 years ago by kini

I see. This is now fixed too. :)

Changed 9 years ago by kini

diff of latest commit in SPKG, for review purposes

comment:5 Changed 9 years ago by jhpalmieri

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.

Changed 9 years ago by kini

apply to $SAGE_ROOT/devel/sage

comment:6 Changed 9 years ago by kini

  • Description modified (diff)

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...

comment:7 Changed 9 years ago by kini

Patchbot: apply trac_13389-path-capitalization.patch

comment:8 Changed 9 years ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

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?

comment:9 Changed 9 years ago by kini

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

comment:10 Changed 9 years ago by kini

Thanks for the review!

comment:11 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.3 to sage-5.4

comment:12 Changed 9 years ago by schilly

the new optional spkg is uploaded to the server+mirrors

comment:13 Changed 9 years ago by jdemeyer

  • Merged in set to sage-5.4.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.