Opened 4 years ago

Closed 4 years ago

#19540 closed enhancement (fixed)

AsymptoticExpansion.factorial

Reported by: dkrenn Owned by:
Priority: major Milestone: sage-7.1
Component: asymptotic expansions Keywords:
Cc: behackl, cheuberg Merged in:
Authors: Daniel Krenn Reviewers: Clemens Heuberger
Report Upstream: N/A Work issues:
Branch: 0e6a0c5 (Commits) Commit: 0e6a0c554fbff9ba6fd36f5309e9ec6e7a8eb7f3
Dependencies: #19306, #19521, #19528, #19944 Stopgaps:

Description

Use existing Stirling approximation to calculate the factorial.

Change History (17)

comment:1 Changed 4 years ago by dkrenn

  • Branch set to u/dkrenn/asy/factorial

comment:2 Changed 4 years ago by dkrenn

  • Commit set to 8858983adc355647a528bbc04f9999bda4e342d9
  • Status changed from new to needs_review

Last 10 new commits:

6e5a098Merge branch 'u/dkrenn/symbolic-subring' of trac.sagemath.org:sage into coerce/inverse-action
5638459#19521: use coercion_reversed in InverseAction
421e377mutable poset map: remove elements ``None``
1d28240term monoid: write change_parameter
2c37889correct a bug in change_parameter
bdcb72bwrite map_coefficients
15865ebMerge branches 'coerce/inverse-action' and 'asy/map_coefficients' into t/19306/asy/generators
94f003afactorial
5cbabc1seealso and links
8858983correct :meth: --> :func:

comment:3 follow-up: Changed 4 years ago by cheuberg

  • Reviewers set to Clemens Heuberger
  • Status changed from needs_review to needs_work

In principle, this would be fine; however, the following example fails:

sage: A.<m> = AsymptoticRing('m^QQ', QQ)
sage: m.factorial()
Traceback (most recent call last):
...
TypeError: ...
>>>>>> *previous* ArithmeticError: Cannot build log(m)
since log(m) is not in Growth Group
m^QQ * (e^(n*log(n)))^QQ * (e^n)^QQ * n^QQ * log(n)^QQ.

The problem is that the method tries to build the correct growth group in n (hard-coded) but has no influence on the given growth group. And if we have something like log(m).factorial(), then everything gets more involved. This either needs to be fixed or documented.

comment:4 Changed 4 years ago by cheuberg

  • Milestone changed from sage-6.10 to sage-7.1

comment:5 Changed 4 years ago by git

  • Commit changed from 8858983adc355647a528bbc04f9999bda4e342d9 to 139f7c8078f6e86549b43860eca815763f9dc160

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

5977604Merge tag '7.1.beta3' into t/19540/asy/factorial
8a43e7eTrac #19540: allow growth groups strings in TermMonoid factory
0384e25Trac #19540: use is_one()
3f0f855Trac #19540: variable_names for growth elements
eb49c65Trac #19540: variable_names for terms
5a4adf0Trac #19540: variable_names for asymptotic expansions
d9f2796Trac #19540: _factorial_ for terms
c801853Trac #19540: rewrite factorial
139f7c8Trac #19540: note on log(s).factorial()

comment:6 in reply to: ↑ 3 Changed 4 years ago by dkrenn

  • Status changed from needs_work to needs_review

Replying to cheuberg:

In principle, this would be fine; however, the following example fails:

sage: A.<m> = AsymptoticRing('m^QQ', QQ)
sage: m.factorial()
Traceback (most recent call last):
...
TypeError: ...
>>>>>> *previous* ArithmeticError: Cannot build log(m)
since log(m) is not in Growth Group
m^QQ * (e^(n*log(n)))^QQ * (e^n)^QQ * n^QQ * log(n)^QQ.

The problem is that the method tries to build the correct growth group in n (hard-coded) but has no influence on the given growth group.

Corrected (needed more rewriting than thought, but now factorial can do much better)

And if we have something like log(m).factorial(), then everything gets more involved. This either needs to be fixed or documented.

Documented.

comment:7 Changed 4 years ago by git

  • Commit changed from 139f7c8078f6e86549b43860eca815763f9dc160 to 23948e43b8038cfe7948fab2f5058cab8f734dbd

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

456d8c3Trac #19969: correct whitespaces
b540598Trac #19969: add an additional doctest
984ca4dMerge branch 'u/dkrenn/asy/SA-generator-log' of git://trac.sagemath.org/sage into t/19944/asy/singularity-analysis-method
e1e2ef3Trac #19944: minor restructure of code
97249e7Trac #19944: minor improvement of docstrings (whitespaces, punctation, ...)
ad62c31Trac #19944: show element in NotImplementedError
9ce7b35Trac #19944: exchange order of parameters var and zeta in _singularity_analysis_
c79a6f7Trac #19944: exchange zeta and var in docstrings
452c43bTrac #19944: _singularity_analysis_ methods: more precise documentation
23948e4Merge branch 'u/cheuberg/asy/singularity-analysis-method' of git://trac.sagemath.org/sage into t/19540/asy/factorial

comment:8 Changed 4 years ago by dkrenn

  • Dependencies changed from #19306, #19521, #19528 to #19306, #19521, #19528, #19944

Merged #19944 due to conflicts.

comment:9 follow-up: Changed 4 years ago by cheuberg

  • Status changed from needs_review to needs_info
  1. The growth elements call self.parent()._var_.variable_names(). I am wondering why they do not simply call self.parent().variable_names().
  2. Why can't a generic growth element decide on its own? It could simply call
    if self.is_one():
       return tuple()
    else:
       return self.parent()._var_.variable_names()
    
    which would probably end in a desaster if called on a generic growth element, but we are not supposed to instantiate generic growth elements anyway ...

The rest is fine for me.

comment:10 in reply to: ↑ 9 Changed 4 years ago by dkrenn

Replying to cheuberg:

  1. The growth elements call self.parent()._var_.variable_names(). I am wondering why they do not simply call self.parent().variable_names().
  2. Why can't a generic growth element decide on its own? It could simply call
    if self.is_one():
       return tuple()
    else:
       return self.parent()._var_.variable_names()
    
    which would probably end in a desaster if called on a generic growth element, but we are not supposed to instantiate generic growth elements anyway ...

1+2: In the way it is it allows to build n.factorial() starting in AsymptoticRing('m^QQ * n^QQ', QQ); self.parent().variable_names() would not allow this.

comment:11 follow-up: Changed 4 years ago by cheuberg

  1. This would not change functionality, but the parent has a method, why not calling it.
  2. The generic implementation proposed here would be usable for all growth groups except products, but those have to implement their own method anyway.

comment:12 Changed 4 years ago by git

  • Commit changed from 23948e43b8038cfe7948fab2f5058cab8f734dbd to 3bd6573796bda4091a17b81bef1ede91e3295318

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

3bd6573Trac #19540: collect and compactify various variable_names methods

comment:13 in reply to: ↑ 11 Changed 4 years ago by dkrenn

  • Status changed from needs_info to needs_review

Replying to cheuberg:

  1. This would not change functionality, but the parent has a method, why not calling it.
  2. The generic implementation proposed here would be usable for all growth groups except products, but those have to implement their own method anyway.

Now I understand. Code rewritten.

comment:14 Changed 4 years ago by cheuberg

  • Branch changed from u/dkrenn/asy/factorial to u/cheuberg/asy/factorial

comment:15 follow-up: Changed 4 years ago by cheuberg

  • Commit changed from 3bd6573796bda4091a17b81bef1ede91e3295318 to 0e6a0c554fbff9ba6fd36f5309e9ec6e7a8eb7f3

I added a doctest because I wanted to know what happens for a generic growth group. At first glance, it was surprising that I do get an AttributeError instead of a NotImplementedError. The method is_one seems to be contributed by the category.

I noticed that the docstring of GenericGrowthGroup (and the other growth groups) says "It has to be a subcategory of Join of Category of groups and Category of posets. This is also the default category if None is specified." which is a relict from the past.

Furthermore, we have

    # set everything up to determine category
    from sage.categories.sets_cat import Sets
    from sage.categories.posets import Posets
    from sage.categories.magmas import Magmas
    from sage.categories.additive_magmas import AdditiveMagmas

in the class GenericGrowthGroup which shows all these Categories in the documentation, similar as #20045.

Shall we fix those in a separate ticket or right here?

Please cross-review my commit and set to positive_review if you are satisfied.


New commits:

0e6a0c5Trac #19540: additional doctest

comment:16 in reply to: ↑ 15 Changed 4 years ago by dkrenn

  • Status changed from needs_review to positive_review

Replying to cheuberg:

I added a doctest because I wanted to know what happens for a generic growth group. At first glance, it was surprising that I do get an AttributeError instead of a NotImplementedError. The method is_one seems to be contributed by the category.

I noticed that the docstring of GenericGrowthGroup (and the other growth groups) says "It has to be a subcategory of Join of Category of groups and Category of posets. This is also the default category if None is specified." which is a relict from the past.

Furthermore, we have

    # set everything up to determine category
    from sage.categories.sets_cat import Sets
    from sage.categories.posets import Posets
    from sage.categories.magmas import Magmas
    from sage.categories.additive_magmas import AdditiveMagmas

in the class GenericGrowthGroup which shows all these Categories in the documentation, similar as #20045.

Shall we fix those in a separate ticket or right here?

This is now #20077.

Please cross-review my commit and set to positive_review if you are satisfied.

Done.

comment:17 Changed 4 years ago by vbraun

  • Branch changed from u/cheuberg/asy/factorial to 0e6a0c554fbff9ba6fd36f5309e9ec6e7a8eb7f3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.