Opened 3 years ago

Closed 3 years ago

#25913 closed enhancement (fixed)

Allow input as packed words for Hopf algebra WQSym

Reported by: alauve Owned by:
Priority: minor Milestone: sage-8.4
Component: combinatorics Keywords: sagedays@icerm, packed words, IMA coding sprint, CHAs
Cc: tscrim, nthiery, darij, zabrocki, alauve, amypang, hmlodecki, saliola Merged in:
Authors: Aaron Lauve Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 8e3b93f (Commits, GitHub, GitLab) Commit: 8e3b93f729fba9d6368b4ab6cfb57d0040afe195
Dependencies: Stopgaps:

Status badges

Description

The Hopf algebra WordQuasiSymmetricFunctions, despite it's name, is indexed internally as ordered set partitions. This ticket aims to allow the user to also use packed words as input.

Change History (26)

comment:1 Changed 3 years ago by alauve

  • Branch set to public/combinat/allow-packed-words-in-wqsym-25913
  • Commit set to 709ae4ce4301f844a2c1f8ea0b4994d94b4494dd

New commits:

481b0f0modify getitem to allow for use of packed words as indices
dcecdecMerge branch 'develop' into public/combinat/allow-packed-words-in-wqsym
709ae4cbegin implementing indexing by packed words

comment:2 Changed 3 years ago by git

  • Commit changed from 709ae4ce4301f844a2c1f8ea0b4994d94b4494dd to faf13e7205bc2c50b66c11a3d91347dca8458800

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

30f38f1fighting with global options
faf13e7moved call to GlobalOptions

comment:3 Changed 3 years ago by git

  • Commit changed from faf13e7205bc2c50b66c11a3d91347dca8458800 to 1302381a1045dba36684dee7fc43b025a601b77d

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

e6256d9Merge branch 'develop' into public/combinat/allow-packed-words-in-wqsym-25913
1302381updated documentation to illustrate the words realization of WQSym

comment:4 Changed 3 years ago by alauve

  • Cc nthiery added
  • Status changed from new to needs_review

Updated documentation to reflect the words realization of WQSym. Should be ready for a close inspection.

comment:5 Changed 3 years ago by tscrim

  • Status changed from needs_review to needs_work

This does not actually look correct. The global options should not be a specific instance but instead constructed as class-level subclass (as it was before; it results in an instance due to some magic IIRC, but that is an implementation detail).

Also, as we discussed at lunch today, we do not want to have two different objects for WQSym (over the same base ring).

comment:6 Changed 3 years ago by alauve

I am not happy with the current (development branch) situation, but also not happy with this branch... worrying about end-user getting unexpected results from misuse of .monomial, .sum_of_terms, etc.

For instance, if users see packed words in their output (e.g., M[1,1,2,1]) and also are allowed to write M[1,1,2,1] as input, they will be inclined to write code like this:

out = M.zero()
for w in PackedWords(4):
    M.sum_of_terms((v,1) for v in w. right_weak_order_greater())

This may only cause confusion once per user, but I'm sure there are more exotic errors I'm not thinking of.

What if we do this?: create a new class called WQSymKeys that has exactly one job: handle, without complaint, either packed words or ordered set partitions and converting as appropriate. Might there be a way to do this that would make the previous code-block and the next one run error-free?:

out = M.zero()
for w in OrderedSetPartitions(4):
    M.sum_of_terms((v,1) for v in op.finer())

comment:7 Changed 3 years ago by tscrim

The default is not packed words, so when you change the display options, you take responsibility for realizing that output is not input. We are trying to make both as possibilities, but there are ambiguities in the input. We can document that we try, but to avoid ambiguities, you will need to have users explicitly cast their objects as packed words, which we can then handle in __getitem__, but you must have read the documentation, where it is clearly spelled out that we are using ordered set partitions as the basis. As we discussed, we can change this, but we have to change everything.

Also, sum_of_terms and the like assumes you are passing in good indices. However, that is a completely separate issue.

Last edited 3 years ago by tscrim (previous) (diff)

comment:8 Changed 3 years ago by alauve

Getting back to this ticket finally...

After discussion at SageDays?@ICERM, it seems I should scrap/revert most of the commits on this ticket.

The new plan (submitted here for your correction or comment, if I have remembered it incorrectly):

  • basis keys used internally are ordered set partitions
  • print output as ordered set partitions unless global option is set to words
  • adjust code so that, e.g., in __getitem__,
    • first look for types (Packed)Word and OrderedSetPartition,
    • next assume the user is passing a (packed) word (as a list),
    • and if that makes no sense, then try to view that list as an ordered set partition.
  • be sure the documentation clearly spells out the above, and that the best way for user to have no surprises to write M[w] or M[pi], where w has type (Packed)Word and pi has type OrderedSetPartition

Here are the ambiguities for the user, so far as I can tell:

  • M[1,2,3] -- will output M[{1},{2},{3}] while the user may have meant M[{1,2,3}]
  • M[[1,2,3]] -- will output M[{1},{2},{3}] while the user may have meant M[{1,2,3}]
  • M[[[1,2,3]]] -- will output M[{1,2,3}] and surely the user did not mean M[{1},{2},{3}]
  • M[1,1,2] -- will correctly output M[{1,2},{3}], while at present it would output M[{1,2}]

comment:9 Changed 3 years ago by git

  • Commit changed from 1302381a1045dba36684dee7fc43b025a601b77d to 8f958cfb2e583cec9e990564d240e97656997a17

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

5590097Revert "updated documentation to illustrate the words realization of WQSym"
06c265eRevert "moved call to GlobalOptions"
ba6e0e8Revert "fighting with global options"
7f584c4Revert "begin implementing indexing by packed words"
8f958cfRevert "modify getitem to allow for use of packed words as indices"

comment:10 Changed 3 years ago by git

  • Commit changed from 8f958cfb2e583cec9e990564d240e97656997a17 to 8cda2ef85a2764504f11aa9336c3d2713b02ad1e

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

8cda2efMerge branch 'develop' into public/combinat/allow-packed-words-in-wqsym-25913

comment:11 Changed 3 years ago by git

  • Commit changed from 8cda2ef85a2764504f11aa9336c3d2713b02ad1e to 60ae680408c74f67757a30cfe2d9c035432319c9

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

60ae680added nonempty tests in set_partitions_ordered; began modifying element constructor within wqsym.py

comment:12 Changed 3 years ago by git

  • Commit changed from 60ae680408c74f67757a30cfe2d9c035432319c9 to b89b5af6b3790e5802ec4b6f14f4d813e3c1bd40

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

931abd1added _element_constructor_ method in wqsym
b89b5afupdating documentation and tests; tracking down bug in set_partition_ordered

comment:13 Changed 3 years ago by alauve

I expect this is ready for review (and certainly ready for others to take a closer peek at).

comment:14 Changed 3 years ago by tscrim

Thank you for working more on this. Here are my comments.

You have to handle zeros better:

sage: WZ = WordQuasiSymmetricFunctions(ZZ)
sage: M = WZ.M()
sage: M(2)
2*M[]
sage: M(0/1)  # This is more common than you might expect
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
...
ValueError: cannot convert 0 into an element of Ordered set partitions

vs current:

sage: M(0/1)
0

Coercion is too strong. Ideally, you should allow conversion of scalars. CombinatorialFreeModule dictates that any 0 that is also in the base_ring() will convert, but since this is known to be an algebra, we should be better and allow the conversion of any scalar.

I don't like the punt-off of __getitem__ to _element_constructor_. If you fix the above issue, than this will work:

sage: WQ = WordQuasiSymmetricFunctions(QQ)
sage: M = WQ.M()
sage: M[0/1]

and result in 0. I think the assumptions you should make between the two will be different. The __getitem__ should assume it is an OSP or packed word and let the error propagate up. I would be okay with the reverse _element_constructor_ calling __getitem__.

These are the same:

sage: [[1,2],]
[[1, 2]]
sage: [[1,2]]
[[1, 2]]

Please remove the extraneous commas in the coproduct()` tests.

comment:15 follow-up: Changed 3 years ago by alauve

Oh my! Do I have things backwards? I thought M(...) should be the catch-all, not M[...]. And I'm happy to handle the zeros better... which of the methods would be most appropriate to handle 0/1?

Actually, looking through the error trace for your test sage: M(0/1), I'm not sure why it failed... , why doesn't 0/1 get picked up by the __call__ or _call_ methods it tries before turning to _element_constructor_?

comment:16 Changed 3 years ago by alauve

  • Cc saliola added

comment:17 in reply to: ↑ 15 Changed 3 years ago by tscrim

Replying to alauve:

Oh my! Do I have things backwards? I thought M(...) should be the catch-all, not M[...]. And I'm happy to handle the zeros better... which of the methods would be most appropriate to handle 0/1?

No, you are correct. However, M[foo] should not return anything that is not a basis element corresponding (under some map) to foo.

Actually, looking through the error trace for your test sage: M(0/1), I'm not sure why it failed... , why doesn't 0/1 get picked up by the __call__ or _call_ methods it tries before turning to _element_constructor_?

Because 0/1 is an element of QQ, which has no coercion into ZZ.

Edit - Forgot to answer your first question.

Last edited 3 years ago by tscrim (previous) (diff)

comment:18 follow-up: Changed 3 years ago by alauve

Hmm. But I view M[...] as attempts to access a basis key. So users have no business trying M[0/1] Or maybe that's your point... I can handle it there (with some careful coding) since it looks so different from what I expect to see?

I have some concern about processing M[0/1] differently than M[2/2] though: I'd like the latter to come in asM[{1}] and not (2/2)*M[], since 2/2 in ZZ evaluates to True.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 3 years ago by tscrim

Replying to alauve:

Hmm. But I view M[...] as attempts to access a basis key. So users have no business trying M[0/1] Or maybe that's your point... I can handle it there (with some careful coding) since it looks so different from what I expect to see?

Yes, they have no business doing it, which is why it should return an error and not silently return a 0. This can make it a lot harder to track down logical bugs.

I have some concern about processing M[0/1] differently than M[2/2] though: I'd like the latter to come in asM[{1}] and not (2/2)*M[], since 2/2 in ZZ evaluates to True.

I agree with you, which is why I do not like the __getitem__ calling _element_constructor_.

comment:20 Changed 3 years ago by git

  • Commit changed from b89b5af6b3790e5802ec4b6f14f4d813e3c1bd40 to 735a4c3050b75b01ef41f3c5145c95a4765cf6ef

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

735a4c3removed _element_constructor_ method

comment:21 Changed 3 years ago by git

  • Commit changed from 735a4c3050b75b01ef41f3c5145c95a4765cf6ef to f40d3e5fdbf66f25099bcd09f5e16f385a8cab35

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

f40d3e5small bug fix

comment:22 in reply to: ↑ 19 Changed 3 years ago by alauve

  • Status changed from needs_work to needs_review

Replying to tscrim:

Replying to alauve:

Hmm. But I view M[...] as attempts to access a basis key. So users have no business trying M[0/1] Or maybe that's your point... I can handle it there (with some careful coding) since it looks so different from what I expect to see?

Yes, they have no business doing it, which is why it should return an error and not silently return a 0. This can make it a lot harder to track down logical bugs.

I have some concern about processing M[0/1] differently than M[2/2] though: I'd like the latter to come in asM[{1}] and not (2/2)*M[], since 2/2 in ZZ evaluates to True.

I agree with you, which is why I do not like the __getitem__ calling _element_constructor_.

Okay. I think I misunderstood what you originally wrote... I thought you wanted M[0/1] to return 0.

I should mention that, since M([[1,3],2]]) worked in the original code (by which I mean M[{1,3},{2}] was returned), I was trying to get M[1,2,1] to work as well. This was the origin of my messing with _element_constructor. But after chatting with others, it seems I shouldn't try to be so accommodating. The basis keys are OSPs, not words. END-OF-STORY.

I removed that custom method, but I could do as you propose and instead let _element_constructor_ pass off to __getitem__. Otherwise, I think this patch is ready for review.

comment:23 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to needs_work
sage: M = algebras.WQSym(QQ).M()
sage: M[2/2]
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-2-e174bca538a0> in <module>()
----> 1 M[Integer(2)/Integer(2)]

/home/uqtscrim/sage-build/local/lib/python2.7/site-packages/sage/structure/parent.pyx in sage.structure.parent.Parent.__getitem__ (build/cythonized/sage/structure/parent.c:11402)()
   1222             except AttributeError:
   1223                 return self.list()[n]
-> 1224         return meth(n)
   1225 
   1226     #################################################################################

/home/uqtscrim/sage-build/local/lib/python2.7/site-packages/sage/combinat/chas/wqsym.pyc in __getitem__(self, p)
   1967                 return self.monomial(self._indices(p))
   1968             except TypeError:
-> 1969                 raise ValueError("cannot convert %s into an element of %s"%(p, self._indices))
   1970 
   1971         def is_field(self, proof=True):

ValueError: cannot convert 1 into an element of Ordered set partitions
sage: M.indices().from_finite_word([2/2])
[{1}]
sage: M[M.indices().from_finite_word([2/2])]
M[{1}]
sage: M[M.indices().from_finite_word([2/2])] == M[1]
True

Don't do isinstance(x, (int, Integer)). Use x in ZZ. Getting rational numbers instead of true integers is a very easy trap to fall into.

I also think it would be good to document explicitly some of the ambiguities and says what it defaults to treating it as when it is ambiguous.

comment:24 Changed 3 years ago by git

  • Commit changed from f40d3e5fdbf66f25099bcd09f5e16f385a8cab35 to 8e3b93f729fba9d6368b4ab6cfb57d0040afe195

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

8e3b93fgetitem tweak; improving documentation

comment:25 Changed 3 years ago by tscrim

  • Status changed from needs_work to positive_review

Thank you. LGTM.

comment:26 Changed 3 years ago by vbraun

  • Branch changed from public/combinat/allow-packed-words-in-wqsym-25913 to 8e3b93f729fba9d6368b4ab6cfb57d0040afe195
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.