Opened 4 years ago

Closed 4 years ago

#25689 closed enhancement (fixed)

Update q_analogues.q_jordan

Reported by: Tomer Bauer Owned by:
Priority: major Milestone: sage-8.3
Component: combinatorics Keywords: days94, q-analogs
Cc: ​tscrim, Jeroen Demeyer Merged in:
Authors: Tomer Bauer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 0c4e9ae (Commits, GitHub, GitLab) Commit: 0c4e9aedded12dc271b84e1cc3521934ed62d7b8
Dependencies: Stopgaps:

Status badges

Description

Small updates to q_jordan:

  1. Add default for q, like the other q-functions.
  2. Better handling of parents.
  3. Allow the q=1 case.
  4. Better documentation.

On the way, an update for the multinomial function was needed.

For future reference, it might be possible to implement q_jordan using some closed-form expression, but that is for another ticket.

Change History (18)

comment:1 Changed 4 years ago by Tomer Bauer

I have not touched the q_stirling_number2 function added in #25235, even though I used the develop branch.

comment:2 Changed 4 years ago by Tomer Bauer

Status: newneeds_review

comment:3 Changed 4 years ago by Tomer Bauer

Cc: ​tscrim added
Summary: Update q_analogs.q_jordanUpdate q_analogues.q_jordan

comment:4 Changed 4 years ago by Jeroen Demeyer

Can't we deprecate the list/iterable input? I don't see the point of that, given that you can just do multinomial(*L) instead of multinomial(L).

comment:5 Changed 4 years ago by Jeroen Demeyer

Status: needs_reviewneeds_info

Why did you remove q_stirling_number2?

comment:6 Changed 4 years ago by Jeroen Demeyer

Allowing general iterables is also dangerous because some Elements are iterable. Imagine for example that we extend this to allowing polynomials...

comment:7 in reply to:  6 Changed 4 years ago by Tomer Bauer

Replying to jdemeyer:

Allowing general iterables is also dangerous because some Elements are iterable. Imagine for example that we extend this to allowing polynomials...

For multinomial I would still like to use at least a list, a tuple, a Partition or a Composition as input. One example is calculating the number of elements of a given cycle type in the symmetric group, and it can come up as any of those objects.

Replying to jdemeyer:

Why did you remove q_stirling_number2?

We might need a git expert to explain how/why q_stirling_number2 was removed. What is the best option to add it back without a new commit? I even fix it a bit in #25715.

comment:8 in reply to:  4 Changed 4 years ago by Tomer Bauer

Replying to jdemeyer:

Can't we deprecate the list/iterable input? I don't see the point of that, given that you can just do multinomial(*L) instead of multinomial(L).

This probably should be done in a new ticket. One point of deprecating the variable amount of arguments version is that it is less consistent with some parts of Sage. For example, Partition and Composition get an iterable, as does Python's builtins such as list, set and tuple.

comment:9 Changed 4 years ago by Tomer Bauer

Cc: Jeroen Demeyer added

comment:10 Changed 4 years ago by Tomer Bauer

Branch: u/mathzeta2/q_jordan_updateu/mathzeta2/q_jordan_update_rebased

comment:11 Changed 4 years ago by Tomer Bauer

Commit: 01334acbdd3ec38f6abaf4cd53dde3f09d9e990751c88c48e07794aee198319a9db84e1a444f4973
Status: needs_infoneeds_review

New commits:

51c88c4Fix q_jordan parent and q=1 case

comment:12 Changed 4 years ago by Travis Scrimshaw

There is not a point of wrapping tp as a Partition in the recursion part as the cache converts it over (you should actually do _Partitions(t) instead of Partition(t) because the former is faster), and then you unwrap it with list.

Otherwise LGTM.

comment:13 Changed 4 years ago by Tomer Bauer

_Partitions is a tiny bit faster indeed. Thanks.

I also thought that the extra call to Partition is redundant, but the recursive call to q_jordan is to the undecorated function. Maybe a new helper function _cached_q_jordan can help?

(A solution for another day is an iterative implementation, as done in q_subgroups_of_abelian_group.)

comment:14 Changed 4 years ago by git

Commit: 51c88c48e07794aee198319a9db84e1a444f49730c4e9aedded12dc271b84e1cc3521934ed62d7b8

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

0c4e9aeUse _Partitions instead of Partition

comment:15 Changed 4 years ago by Tomer Bauer

Now things are better, and use _Partitions, also for q_subgroups_of_abelian_group.

The cached_function decorator use the key argument only for comparisons in the cache, and that is why I was puzzled when checking for the empty partition, as opposed to a list composed of all zeros. The empty partition have zero length and it is False as a boolean:

sage: Partition([]) == Partition([0]) == Partition([0, 0])
True

But this is not true for lists like [0], of course.

comment:16 in reply to:  12 Changed 4 years ago by Tomer Bauer

Replying to tscrim:

Otherwise LGTM.

I wanted to set the ticket to positive review myself, but I would appreciate a look at the last commit for a final OK.

comment:17 Changed 4 years ago by Travis Scrimshaw

Reviewers: Travis Scrimshaw
Status: needs_reviewpositive_review

Sorry, I lost track of this. Your changes are good. Thanks.

comment:18 Changed 4 years ago by Volker Braun

Branch: u/mathzeta2/q_jordan_update_rebased0c4e9aedded12dc271b84e1cc3521934ed62d7b8
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.