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 MacMahon's Omega operator #22066

Closed
dkrenn opened this issue Dec 16, 2016 · 56 comments
Closed

implement MacMahon's Omega operator #22066

dkrenn opened this issue Dec 16, 2016 · 56 comments

Comments

@dkrenn
Copy link
Contributor

dkrenn commented Dec 16, 2016

MacMahon's Omega operator takes a quotient of laurent polynomials and removes all negative exponents in the corresponding power series.

The aim of this ticket is to implement it.

Depends on #21832
Depends on #21833
Depends on #21966
Depends on #21968
Depends on #21976
Depends on #21999
Depends on #22064

CC: @cheuberg

Component: algebra

Author: Daniel Krenn

Branch/Commit: 482ab70

Reviewer: Clemens Heuberger

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

@dkrenn dkrenn added this to the sage-7.5 milestone Dec 16, 2016
@dkrenn
Copy link
Contributor Author

dkrenn commented Dec 16, 2016

Branch: u/dkrenn/omega

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2016

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

d33a595minor rewrite to increase readability
5a5db0ballow passing mon in _element_constructor_
3a809a4fix pickling bug
be67f37determine parent at top of _element_constructor_
6137662rewrite to detect more specialized situations
0570355Merge branch 'u/dkrenn/laurent-efficient-construction' of trac.sagemath.org:sage into u/dakrenn/omega
0f88065quick-fix in normalize
c0fea6eMerge branch 'u/dkrenn/laurent-floor-hash' of trac.sagemath.org:sage into u/dakrenn/omega
9389e62use `__call__` in subs to be more efficient
004d61dMerge branch 'u/dkrenn/laurent-subs' of trac.sagemath.org:sage into u/dakrenn/omega

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2016

Commit: 004d61d

@dkrenn
Copy link
Contributor Author

dkrenn commented Dec 16, 2016

Dependencies: #21832, #21833, #21833, #21966, #21968, #21976, #21999, #22064

@dkrenn
Copy link
Contributor Author

dkrenn commented Dec 16, 2016

Author: Daniel Krenn

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2016

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

41b1dcdlazy import Omega
58943d6rename Omega_higher --> Omega_ge
49e75f4fix imports in doctests
b122d0frename to homogenous_symmetric_function
0b6dba8name Omega_numerator_P private
907d12edocstring and tests for _Omega_numerator_P_
91ee1d0add "helper function" comment to some docstrings
c97bac4fix doc output
b085ca6module description
4b1c37ereferences

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2016

Changed commit from 004d61d to 4b1c37e

@fchapoton
Copy link
Contributor

comment:8

see also #10669

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2016

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

79fe3b3extend docstring at top of file

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 16, 2016

Changed commit from 4b1c37e to 79fe3b3

@dkrenn
Copy link
Contributor Author

dkrenn commented Dec 16, 2016

comment:10

Replying to @fchapoton:

see also #10669

Thank you for mentioning this. (I have never thought anyone already created a ticket for this.) ;)

@cheuberg
Copy link
Contributor

Changed dependencies from #21832, #21833, #21833, #21966, #21968, #21976, #21999, #22064 to #21832, #21833, #21966, #21968, #21976, #21999, #22064

@cheuberg
Copy link
Contributor

comment:11

Removed duplicate dependency #21833

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2016

Changed commit from 79fe3b3 to 82327c8

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 22, 2016

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

f7e9540fix doctest (conversion to symbolic)
82327c8Merge branch 'u/dakrenn/laurent-subs' into t/22066/omega

@dkrenn
Copy link
Contributor Author

dkrenn commented Dec 22, 2016

comment:13

Merged in changed dependencies.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 26, 2016

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

f046132Python3 iteritems
0db5399MPolynomial: correct coefficient ring when dict as input
a20fb8arevert 0f88065556 quick-fix in normalize
397301adoctest example of ticket 21999
fceb3e1Merge branch 't/21999/laurent-floor-hash' into t/22066/omega

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Dec 26, 2016

Changed commit from 82327c8 to fceb3e1

@cheuberg

This comment has been minimized.

@cheuberg
Copy link
Contributor

cheuberg commented Jan 2, 2017

comment:15

This is a review of this ticket modulo its dependencies.

General:

  1. I am not happy to have Omega in the global name space. There might be other uses of the upper case greek letter Omega which might as well live in the global name space, e.g., in context with asymptotics. Thus I suggest to have it as MacMahonOmega or something like that, perhaps with an alias in the docstring.

  2. Is there a reason why some of the auxiliary functions are visible and some others are hidden (e.g., for generating function of integral points of polyhedra #22067?). In that case, I suggest to have one example in the module documentation which demonstrates how to use Omega and perhaps one other example there which demonstrates how to use the other functions efficiently.

  3. I think that "laurent" should be capitalized throughout, cf. the Wikipedia article.

  4. Nowadays, the developers guide wants us to store all bibliographical references in SAGE_ROOT/src/doc/en/reference/references/index.rst.

  5. Long descriptions in docstrings: The developer guide does not give clear indications; nevertheless, I suggest to use the same command form as in the short description block above. Otherwise, it seems to be strange to have "Return ..." ... "this calculates".

Function Omega:

  1. expression is allowed to be a Factorization (as per the Tests), but this is not documented.

  2. Add examples demonstrating all allowed input variants (expression only, without denominator; denominator as a non-factorized Polynomial); perhaps use a simple example to demonstrate all useful variations.

  3. There are two consecutive lines of imports from sage.rings.polynomial.laurent_polynomial_ring which could be done in one import; the second line is too long anyway

  4. Document and test NotImplementedError for op != operator.ge.

  5. Test ValueError for not denominator.is_integral().

  6. The line numerator *= denominator.unit() seems to be suspicious. Shouldn't it be /=?

  7. Test ValueError "... is not a variable".

  8. Test ZeroDivisionError

  9. Test NotImplementedError "... is not normalized"

  10. Test NotImplementedError "Cannot handle factor"

Function _Omega_:

  1. Although main testing is done in Omega, it would be nice to have one non-trivial example to see how the input and output looks like.

Function Omega_ge:

  1. State in docstring that z_1, ..., z_n do not appear in the input, but do appear in the output.

  2. State the parent of the output in the docstring (The function _Omega_ uses the fact that the parent does not depend on a and uses a specific order of the variables)

  3. Is there a reason not to use CyclotomicField?

  4. Is there a reason to actually give the number of variables in LaurentPolynomialRing?

  5. subs_power: this is never called with value != None but is only documented with value != None. Thus remove the parameter value and adapt documentation.

  6. subs_power: The line p = tuple(var.dict().popitem()[0]).index(1) is rather hard to understand, document that this actually just means that var is the pth generator of the parent.

  7. subs_power: also state that it is assumed that var only occurs with exponents divisible by exponent

Function Omega_numerator:

  1. Document that the numerator is normalized in such a way that the corresponding denominator has constant term 1.

  2. In Omega_numerator it is not clear why the input is not assumed to be flattened beforehand. Add a pointer to Omega_denumerator.

Function _Omega_numerator_P:

  1. When reading [APR2001] one would assume that _Omega_numerator_P_ should be a cached function. I assume that performance tuning showed that this does not help. Document this.

  2. Add a meaningful description; currently, one must find out that these are the polynomials P in [APR2001].

  3. The last component of the input x is never used; t is used instead. Consider removing the last component of x. Explain the situation.

Function Omega_factors_denominator:

  1. Document that the denominator is normalized such that it has constant term 1 (cf. 24)

  2. The implicit assumption is that the x and y are collected in such a way that one entry of x corresponds to the orbit of some x_j under multiplication by dth roots of unity and that the output is collected in a corresponding way.

Function partition:

  1. Perhaps use an "ALGORITHM" block for the link to the source code.

Function homogeneous_symmetric_function:

  1. Give definition (or a link to a definition) of homogeneous symmetric polynomials

  2. "whose type is determined by the input x": what does this mean?

@cheuberg
Copy link
Contributor

cheuberg commented Jan 2, 2017

Reviewer: Clemens Heuberger

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 4, 2017

Changed commit from fceb3e1 to 8ede01a

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 4, 2017

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

d9514a4Trac #22066.14: test non-normalized input
10900a9Trac #22066.15: test non-binom input
c0bf0b5Trac #22066.16: add non-trivial doctest in _Omega_
e60311aTrac #22066.17: document z_i in Omega_ge
80125f8Trac #22066.18: state parent of output in Omega_ge
2a0f48eTrac #22066.19: use CyclotomicField instead of QQ.extension(...)
8f6ec42Trac #22066.21: remove parameter value of subs_power as not needed
d874fd3Trac #22066.22: comment hard to read line
001dccfTrac #22066.23: make a note in docstring on assumptions in subs_power
8ede01aTrac #22066.24: note in Omega_numerator on normalization

@cheuberg
Copy link
Contributor

comment:35

ok. Dependencies are rather orthogonal to this ticket, so I set this one to positive. At some stage, it might be good to implement other algorithms (cf. #10669) and to compare speed.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 9, 2017

Changed commit from 4b1bb1d to f1e2fcd

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Feb 9, 2017

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

fded046Merge branch 'u/dkrenn/laurent-uni-convert' in 7.5.1
c75d370trac 21855 some details
11707edtrac 21855 more details
65e2fc5Merge tag '7.6.beta2' into t/21855/public/21855
80eea43Merge branch 'public/21855' of git://trac.sagemath.org/sage into t/21976/laurent-efficient-construction
f1e2fcdMerge branch 'u/dkrenn/laurent-efficient-construction' of git://trac.sagemath.org/sage into t/22066/omega-7.5.rc1

@dkrenn
Copy link
Contributor Author

dkrenn commented Feb 9, 2017

comment:37

Merged in #21976 (and thus 7.6.beta2) to make this patch work again.

@tscrim
Copy link
Collaborator

tscrim commented Mar 2, 2017

comment:39

Needs another rebasing.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 2, 2017

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

cf22ee8Merge branch 'u/dkrenn/laurent-efficient-construction' of git://trac.sagemath.org/sage into public/polynomials/multivariable_laurent_poly_constructor-21976
c58559dSome reviewer changes.
1b7377cMerge branch 'develop' into public/polynomials/laurent_mpoly_constructor-21976
482ab70Merge branch 'public/polynomials/laurent_mpoly_constructor-21976' of git://trac.sagemath.org/sage into t/22066/omega-7.5.rc1

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Mar 2, 2017

Changed commit from f1e2fcd to 482ab70

@dkrenn
Copy link
Contributor Author

dkrenn commented Mar 2, 2017

comment:41

Replying to @tscrim:

Needs another rebasing.

Merged in dependency #21976. Once the patchbot is happy, this can be positive again.

@vbraun
Copy link
Member

vbraun commented Mar 5, 2017

Changed branch from u/dkrenn/omega-7.5.rc1 to 482ab70

@vbraun vbraun closed this as completed in 4ca33ea Mar 5, 2017
kryzar pushed a commit to kryzar/sage that referenced this issue Feb 6, 2023
Implement this using !MacMahon's Omega operator (sagemath#22066).

URL: https://trac.sagemath.org/22067
Reported by: dkrenn
Ticket author(s): Daniel Krenn
Reviewer(s): Matthias Koeppe
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

5 participants