Opened 5 years ago
Closed 5 years ago
#19540 closed enhancement (fixed)
AsymptoticExpansion.factorial
Reported by:  dkrenn  Owned by:  

Priority:  major  Milestone:  sage7.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, GitHub, GitLab)  Commit:  0e6a0c554fbff9ba6fd36f5309e9ec6e7a8eb7f3 
Dependencies:  #19306, #19521, #19528, #19944  Stopgaps: 
Description
Use existing Stirling approximation to calculate the factorial.
Change History (17)
comment:1 Changed 5 years ago by
 Branch set to u/dkrenn/asy/factorial
comment:2 Changed 5 years ago by
 Commit set to 8858983adc355647a528bbc04f9999bda4e342d9
 Status changed from new to needs_review
comment:3 followup: ↓ 6 Changed 5 years ago by
 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
(hardcoded) 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 5 years ago by
 Milestone changed from sage6.10 to sage7.1
comment:5 Changed 5 years ago by
 Commit changed from 8858983adc355647a528bbc04f9999bda4e342d9 to 139f7c8078f6e86549b43860eca815763f9dc160
Branch pushed to git repo; I updated commit sha1. New commits:
5977604  Merge tag '7.1.beta3' into t/19540/asy/factorial

8a43e7e  Trac #19540: allow growth groups strings in TermMonoid factory

0384e25  Trac #19540: use is_one()

3f0f855  Trac #19540: variable_names for growth elements

eb49c65  Trac #19540: variable_names for terms

5a4adf0  Trac #19540: variable_names for asymptotic expansions

d9f2796  Trac #19540: _factorial_ for terms

c801853  Trac #19540: rewrite factorial

139f7c8  Trac #19540: note on log(s).factorial()

comment:6 in reply to: ↑ 3 Changed 5 years ago by
 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
(hardcoded) 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 5 years ago by
 Commit changed from 139f7c8078f6e86549b43860eca815763f9dc160 to 23948e43b8038cfe7948fab2f5058cab8f734dbd
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
456d8c3  Trac #19969: correct whitespaces

b540598  Trac #19969: add an additional doctest

984ca4d  Merge branch 'u/dkrenn/asy/SAgeneratorlog' of git://trac.sagemath.org/sage into t/19944/asy/singularityanalysismethod

e1e2ef3  Trac #19944: minor restructure of code

97249e7  Trac #19944: minor improvement of docstrings (whitespaces, punctation, ...)

ad62c31  Trac #19944: show element in NotImplementedError

9ce7b35  Trac #19944: exchange order of parameters var and zeta in _singularity_analysis_

c79a6f7  Trac #19944: exchange zeta and var in docstrings

452c43b  Trac #19944: _singularity_analysis_ methods: more precise documentation

23948e4  Merge branch 'u/cheuberg/asy/singularityanalysismethod' of git://trac.sagemath.org/sage into t/19540/asy/factorial

comment:8 Changed 5 years ago by
 Dependencies changed from #19306, #19521, #19528 to #19306, #19521, #19528, #19944
Merged #19944 due to conflicts.
comment:9 followup: ↓ 10 Changed 5 years ago by
 Status changed from needs_review to needs_info
 The growth elements call
self.parent()._var_.variable_names()
. I am wondering why they do not simply callself.parent().variable_names()
.  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 5 years ago by
Replying to cheuberg:
 The growth elements call
self.parent()._var_.variable_names()
. I am wondering why they do not simply callself.parent().variable_names()
. 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 followup: ↓ 13 Changed 5 years ago by
 This would not change functionality, but the parent has a method, why not calling it.
 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 5 years ago by
 Commit changed from 23948e43b8038cfe7948fab2f5058cab8f734dbd to 3bd6573796bda4091a17b81bef1ede91e3295318
Branch pushed to git repo; I updated commit sha1. New commits:
3bd6573  Trac #19540: collect and compactify various variable_names methods

comment:13 in reply to: ↑ 11 Changed 5 years ago by
 Status changed from needs_info to needs_review
Replying to cheuberg:
 This would not change functionality, but the parent has a method, why not calling it.
 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 5 years ago by
 Branch changed from u/dkrenn/asy/factorial to u/cheuberg/asy/factorial
comment:15 followup: ↓ 16 Changed 5 years ago by
 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 crossreview my commit and set to positive_review if you are satisfied.
New commits:
0e6a0c5  Trac #19540: additional doctest

comment:16 in reply to: ↑ 15 Changed 5 years ago by
 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 aNotImplementedError
. The methodis_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 AdditiveMagmasin 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 crossreview my commit and set to positive_review if you are satisfied.
Done.
comment:17 Changed 5 years ago by
 Branch changed from u/cheuberg/asy/factorial to 0e6a0c554fbff9ba6fd36f5309e9ec6e7a8eb7f3
 Resolution set to fixed
 Status changed from positive_review to closed
Last 10 new commits:
Merge branch 'u/dkrenn/symbolicsubring' of trac.sagemath.org:sage into coerce/inverseaction
#19521: use coercion_reversed in InverseAction
mutable poset map: remove elements ``None``
term monoid: write change_parameter
correct a bug in change_parameter
write map_coefficients
Merge branches 'coerce/inverseaction' and 'asy/map_coefficients' into t/19306/asy/generators
factorial
seealso and links
correct :meth: > :func: