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

Implement global options for WQSym #25155

Closed
alauve opened this issue Apr 12, 2018 · 43 comments
Closed

Implement global options for WQSym #25155

alauve opened this issue Apr 12, 2018 · 43 comments

Comments

@alauve
Copy link

alauve commented Apr 12, 2018

Ordered set partitions are in bijection with packed words. It would be nice to be able to work with packed words in the Hopf algebra WQSym. In the branch, I shall also implement "tight" and "compact" notations for the output.

Depends on #25133

CC: @alauve @tscrim @darijgr @amypang @zabrocki

Component: combinatorics

Keywords: IMA coding sprint, CHAs

Author: Aaron Lauve

Branch: c2ffc13

Reviewer: Travis Scrimshaw

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

@alauve alauve added this to the sage-8.2 milestone Apr 12, 2018
@alauve
Copy link
Author

alauve commented Apr 12, 2018

Branch: public/combinat/wqsym_options-25155

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 12, 2018

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

8a36664documentation fixes
b4a3af9implement WQSym -> QSym projection
fc6b2aefix to_quasisymmetric_function
f75e10dif w1 is a word, return a word, else a list
3f8f272added doc test, minor change for test of Composition, correct import
06fd19dMerge branch 'public/25018/bug_in_shuffle' of git://trac.sagemath.org/sage into public/combinat/extend_shuffle_product-15597
4252484Merge branch 'public/combinat/extend_shuffle_product-15597' of git://trac.sagemath.org/sage into public/combinat/implement_wqsym-25133
82240b2Fixing some details and making Symmetric capitalized in the name.
9d90234Initial additional of global options to WQSym
fd61441Added two global options for display of elements of WordQuasiSymmetricFunctions.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 12, 2018

Commit: fd61441

@tscrim
Copy link
Collaborator

tscrim commented Apr 12, 2018

Changed dependencies from 25133 to #25133

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 13, 2018

Changed commit from fd61441 to c828b0e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 13, 2018

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

c828b0eSome reviewer changes.

@tscrim
Copy link
Collaborator

tscrim commented Apr 13, 2018

comment:5

If my changes look good, then positive review.

@tscrim
Copy link
Collaborator

tscrim commented Apr 13, 2018

Reviewer: Travis Scrimshaw

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 13, 2018

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

ba70317A few more reviewer changes.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 13, 2018

Changed commit from c828b0e to ba70317

@alauve
Copy link
Author

alauve commented Apr 26, 2018

comment:8

I'm happy with these changes. How do others feel about the following snippet from OrderedSetPartition ?:

    INPUT:

    - ``parts`` -- the parts of the ordered set partition
    - ``from_word`` -- (optional) allows the creation of an ordered set
      partition from any finite word or iterable , e.g., a packed word

The reader may be led to think that parts is required. In fact, the way the code is written, exactly one of parts and from_word is used (and it's from_word if that one is given). It must be a keyword/named argument, as the code is currently written, because I was being lazy about how to distinguish between the two possible inputs. E.g., if the lone argument [[1,2],[4],[3]] is passed, should this be viewed as a word on the alphabet "finite subsets of NN" or as a partition of {1,2,3,4} into blocks? Probably the latter, but should I make that restriction on future use?

@darijgr
Copy link
Contributor

darijgr commented Apr 26, 2018

comment:9

Just say "(in this case, parts needs not be provided)"?

@tscrim
Copy link
Collaborator

tscrim commented Apr 27, 2018

comment:10

The other way would be to say "one of the following must be provided" but that is slightly misleading as parts could be a packed word IIRC. Thinking a little more, perhaps this is better:

INPUT

- ``parts`` -- an object or iterable that defines an ordered set partition;
  if there is ambiguity, use the keyword ``from_word`` if the input should
  be treated as a packed word

@darijgr
Copy link
Contributor

darijgr commented Apr 27, 2018

comment:11

Argh! Do we really want duck-typing here? Please think hard about whether this is really unambiguous and whether it's worth the slowdown.

@tscrim
Copy link
Collaborator

tscrim commented Apr 27, 2018

comment:12

Yes, this is the behavior we want. It is stupid and annoying to make users have to use a keyword argument when a packed word as a list is sufficient. The ambiguity is when you want a packed word whose letters are sets/lists/etc. (This is also not ducktyping, but parsing input. Ducktyping is doing something like L[0], where L could be a list/tuple/iterable and gets treated in the same way.)

@alauve
Copy link
Author

alauve commented May 1, 2018

comment:13

So what is better for __classcall_private__? Trying to process an ordered set partition first, or trying to process a packed word first?

At present, the code first tries to process part as a packed word:

        # if `parts` looks like a sequence of "letters" then treat it like a word.
        if parts in Words() or (len(parts) > 0 and isinstance(parts[0], (str, int, Integer))):
            return OrderedSetPartitions().from_finite_word(Words()(parts))
        else:
            P = OrderedSetPartitions( reduce(lambda x,y: x.union(y), map(Set, parts), Set([])) )
            return P.element_class(P, parts)

but this might be better:

        # if `parts` looks like a sequence of "sets" then treat it like an ordered set partition.
        if parts in OrderedSetPartitions() or (len(parts) > 0 and isinstance(parts[0], (list, tuple, set, Set))):
            P = OrderedSetPartitions( reduce(lambda x,y: x.union(y), map(Set, parts), Set([])) )
            return P.element_class(P, parts)
        else:
            return OrderedSetPartitions().from_finite_word(Words()(parts))

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 1, 2018

Changed commit from ba70317 to 6e50492

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 1, 2018

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

6e50492updated doc string for intended usage vis.a.vis packed words

@tscrim
Copy link
Collaborator

tscrim commented May 2, 2018

comment:15

Replying to @alauve:

So what is better for __classcall_private__? Trying to process an ordered set partition first, or trying to process a packed word first?

I don't think it matters. Actually, it only treats words in (str, int, Integer) (not even rationals; a more robust check is x in ZZ modulo str). At least, I wouldn't worry about it until there is a definite problem.

@alauve
Copy link
Author

alauve commented May 4, 2018

comment:16

I don't quite know what you mean by "modulo str." Are you suggesting changes?

Additionally, are there any more changes from #25133 that need to be brought in, or is this good to go?

@tscrim
Copy link
Collaborator

tscrim commented May 5, 2018

comment:17

I am suggesting a change: isinstance(parts[0], (str, int, Integer)) -> parts[0] in ZZ or isinstance(parts[0], str). The "modulo str" was because that is still a useful check.

There is also a doctest in combinat/words/finite_word.py needing a trivial fix (see the patchbot report).

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 6, 2018

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

27b9a98doctest and Travis' check

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 6, 2018

Changed commit from 6e50492 to 27b9a98

@tscrim
Copy link
Collaborator

tscrim commented May 6, 2018

comment:19

Thanks. LGTM.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 9, 2018

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

bc276ddMerge branch 'public/combinat/implement_wqsym-25133' of git://trac.sagemath.org/sage into public/combinat/implement_wqsym-25133
5b2dc1bMerge branch 'public/combinat/wqsym_options-25155' of git://trac.sagemath.org/sage into public/combinat/wqsym_options-25155

@tscrim
Copy link
Collaborator

tscrim commented May 9, 2018

comment:22

Trivial rebase.

@vbraun
Copy link
Member

vbraun commented May 17, 2018

comment:23

PDF docs don't build

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 17, 2018

Changed commit from 5b2dc1b to 77ae3be

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 17, 2018

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:

ef1f3b7link FQSym doc
98fca9cMerge branch 'public/combinat/fqsym2' of trac.sagemath.org:sage into homs
416a8c0more corrections due to recent merge
4f2ddfeMerge branch 'public/combinat/fqsym2' of git://trac.sagemath.org/sage into public/combinat/fqsym2
beeea6aAbstracting methods to DRY.
6e40666Calling the M basis the monomial basis.
8dc0c5cMerge branch 'public/combinat/fqsym2' of git://trac.sagemath.org/sage into 25136
41a17a8Merge branch 'public/combinat/fqsym2' of git://trac.sagemath.org/sage into develop
c5be663Merge branch 'public/combinat/wqsym_options-25155' of git://trac.sagemath.org/sage into 25155
77ae3befix broken latex in doc

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 17, 2018

Changed commit from 77ae3be to c2ffc13

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 17, 2018

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5e85f91Merge branch 'develop' of git://trac.sagemath.org/sage into HEAD
c2ffc13fix broken latex in doc

@darijgr
Copy link
Contributor

darijgr commented May 17, 2018

comment:26

This should do it. (Sorry for forced push -- I had a dirty develop branch, and it smuggled several other tickets in this.)

@vbraun
Copy link
Member

vbraun commented May 19, 2018

Changed branch from public/combinat/wqsym_options-25155 to c2ffc13

@alauve
Copy link
Author

alauve commented Jun 14, 2018

Changed commit from c2ffc13 to none

@alauve
Copy link
Author

alauve commented Jun 14, 2018

comment:28

In my view, this ticket is incomplete. Notice the following:

sage: M = WordQuasiSymmetricFunctions(QQ).M()
sage: c = OrderedSetPartition([1,1,2,1]); c # enter an OSP as a packed word
[{1, 2, 4}, {3}]
sage: M[c]
M[{1, 2, 4}, {3}]
sage: m = M[[1,2,4],[3]]; m
M[{1, 2, 4}, {3}]
sage: M.options.objects = "words"
sage: m
M[1, 1, 2, 1]
sage: M[1,1,2,1], M[[1,1,2,1]]
(M[{1, 2}], M[{1, 2}])

I'd like to see things like M[1,1,2,1] and/or M[[1,1,2,1]] return the basis element corresponding to the indicated packed word, i.e., M[1, 1, 2, 1].

If anybody thinks this is worth a new ticket, perhaps they could weigh-in, after taking a peek at this branch:
public/combinat/allow-packed-words-in-wqsym

@darijgr
Copy link
Contributor

darijgr commented Jun 16, 2018

comment:29

I'm not sure I want global options to modify anything other than output. Imagine, for example, that some code internally calls M[O] for some ordered set partition O. But the user has set a global option that causes this O to be interpreted (wrongly) as a packed word. Hilarity ensues. (We had this problem with the infamous global option that decides in which order permutations are multiplied.)

If you want a quick way to turn a packed word into a basis element of WQSym, define a helper function, like

def Mw(w):
    return M[OrderedSetPartition(w)]

Maybe this should be suggested in the doc.

@tscrim
Copy link
Collaborator

tscrim commented Jun 17, 2018

comment:30

Replying to @darijgr:

(We had this problem with the infamous global option that decides in which order permutations are multiplied.)

I think this is still "have", but I don't have a concrete issue.

I agree with Darij on this, it should only affect the printing output as a convenience for the user.

Actually, why the packed word input does not work (which at least M[1,1,2,1] I think should) is because this fails:

sage: OSP = OrderedSetPartitions()
sage: OSP([1,1,2,1])
...
TypeError: Element has no defined underlying set

So we might want to consider moving the checking to see if the input-is-a-word to the _element_constructor_.

@alauve
Copy link
Author

alauve commented Jun 18, 2018

comment:31

Replying to @tscrim:

Actually, why the packed word input does not work (which at least M[1,1,2,1] I think should) is because this fails:

sage: OSP = OrderedSetPartitions()
sage: OSP([1,1,2,1])
...
TypeError: Element has no defined underlying set

So we might want to consider moving the checking to see if the input-is-a-word to the _element_constructor_.

.

This seems like a good idea to me. I can start a ticket to make this happen. However, ...

I wonder what will/should happen when the user enters M[1,2,3]. Right now, I believe one sees M[{1,2,3}]. Are we all in agreement that one should instead see M[{1}, {2}, {3}]? And that we should demand the user enter M[[1,2,3]] to get the former?

@tscrim
Copy link
Collaborator

tscrim commented Jun 19, 2018

comment:32

Python will not fundamentally distinguish between M[1,2,3] and M[[1,2,3]] other than in the former case, it will be a tuple rather than a list as input. I don't think we want to treat tuples and lists differently, but maybe we do want to do that.

@alauve
Copy link
Author

alauve commented Jun 19, 2018

comment:33

Yeah..., came to that realization after working on the aforementioned branch for awhile this afternoon.
I hate to have M[[1,2,3]] get interpreted as M[{1}, {2}, {3}] (and equally hate asking the user to enter M[[[1,2,3]]]).

If we avoid making a distinction between tuples and lists, should we then drop this idea altogether? So, in documentation, we suggest users build the Mw shortcut of Darij?

@tscrim
Copy link
Collaborator

tscrim commented Jun 19, 2018

comment:34

Quite possibly. Another option I just thought of is to construct a custom basis method that takes a parameter indexed_by_words, where the default is False. If True, then it returns a basis such that it goes through the OrderedSetPartitions.from_word.

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