MareoRaft / k_combinat_for_sage

k-Schur combinatorics for SageMath
4 stars 1 forks source link

RootIdeals.init_parabolic_from_composition does not accept Composition object #10

Closed ghseeli closed 6 years ago

ghseeli commented 6 years ago

Observe the following code

sage: RI = RootIdeals()
sage: RI.init_parabolic_from_composition([1,1]) #good
[(0, 1)]
sage: RI..init_parabolic_from_composition(Composition([1,1])) #bad
Error in lines 1-5
Traceback (most recent call last):
  File "/usr/local/sage/local/lib/python2.7/site-packages/smc_sagews/sage_server.py", line 1044, in execute
    exec compile(block+'\n', '', 'single', flags=compile_flags) in namespace, locals
  File "", line 4, in <module>
  File "k_combinat_for_sage/root_ideal.py", line 717, in init_parabolic_from_composition
    n = sum(composition)
  File "/usr/local/sage/local/lib/python2.7/site-packages/sage/misc/functional.py", line 575, in symbolic_sum
    return expression.sum(*args, **kwds)
TypeError: sum() takes exactly 1 argument (0 given)

Of course, it is quite annoying that sum(Composition([1,1])) does not work (maybe this should be fixed upstream), but for now the error stands.

ghseeli commented 6 years ago

As a follow-up on this, it is somewhat annoying because the Composition class has a static method called sum which concatenates compositions. However, it does not even work the way you would want (because it is a static method)!

sage: c1 = Composition([1,1])
sage: c2 = Composition([2,3])
sage: c1.sum([c2])
[2, 3]

Furthermore, the more intuitive way of doing the same thing already exists and its source code is not even dependent on the sum method detailed above.

sage: c1+c2
[1, 1, 2, 3]

I do think I should submit a trac ticket about this, because it seems like an odd method that does not behave consistently with everything else. Thoughts?

MareoRaft commented 6 years ago

Well I think the recommended syntax would be Composition.sum(c1, c2).

With your syntax and desired output, I would expect the method to be called add or plus, not sum.

That being said, I feel like this should be called concat.

I haven't looked deeply into this, but I trust you have good judgement.

MareoRaft commented 6 years ago

I'm going to focus on the issue in this repo now. I'll make some commits soon.

MareoRaft commented 6 years ago

Feel free to reopen if you have further issues.