search

sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.4k stars 473 forks source link

AsymptoticExpansion.factorial #19540

Closed dkrenn closed 8 years ago

dkrenn commented 8 years ago

Use existing Stirling approximation to calculate the factorial.

Depends on #19306 Depends on #19521 Depends on #19528 Depends on #19944

CC: @behackl @cheuberg

Component: asymptotic expansions

Author: Daniel Krenn

Branch/Commit: 0e6a0c5

Reviewer: Clemens Heuberger

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

dkrenn commented 8 years ago

Branch: u/dkrenn/asy/factorial

dkrenn commented 8 years ago

Commit: 8858983

dkrenn commented 8 years ago

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:
cheuberg commented 8 years ago
comment:3

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.

cheuberg commented 8 years ago

Reviewer: Clemens Heuberger

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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()
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 8858983 to 139f7c8

dkrenn commented 8 years ago
comment:6

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.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 139f7c8 to 23948e4

dkrenn commented 8 years ago
comment:8

Merged #19944 due to conflicts.

dkrenn commented 8 years ago

Changed dependencies from #19306, #19521, #19528 to #19306, #19521, #19528, #19944

cheuberg commented 8 years ago
comment:9
  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.

dkrenn commented 8 years ago
comment:10

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.

cheuberg commented 8 years ago
comment:11
  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.
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 23948e4 to 3bd6573

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

3bd6573Trac #19540: collect and compactify various variable_names methods
dkrenn commented 8 years ago
comment:13

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.

cheuberg commented 8 years ago

Changed branch from u/dkrenn/asy/factorial to u/cheuberg/asy/factorial

cheuberg commented 8 years ago

Changed commit from 3bd6573 to 0e6a0c5

cheuberg commented 8 years ago
comment:15

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
dkrenn commented 8 years ago
comment:16

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.

vbraun commented 8 years ago

Changed branch from u/cheuberg/asy/factorial to 0e6a0c5