Opened 7 years ago
Closed 7 years ago
#18587 closed enhancement (fixed)
cartesian products of growth groups
Reported by: | dkrenn | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.9 |
Component: | asymptotic expansions | Keywords: | asymptotics, gsoc15 |
Cc: | behackl, cheuberg | Merged in: | |
Authors: | Benjamin Hackl, Daniel Krenn | Reviewers: | Clemens Heuberger |
Report Upstream: | N/A | Work issues: | |
Branch: | 74547e6 (Commits, GitHub, GitLab) | Commit: | 74547e6aac076261a6f7da6577d771eb9a830e60 |
Dependencies: | #18223, #18930 | Stopgaps: |
Description (last modified by )
Change History (32)
comment:1 Changed 7 years ago by
- Cc behackl cheuberg added
- Type changed from PLEASE CHANGE to enhancement
comment:2 Changed 7 years ago by
- Branch changed from u/dkrenn/asy/growth-group-cartesian to u/behackl/asy/growth-group-cartesian
- Commit changed from c35bd864269ad55baac18c649741fa445aa6cf4f to 327d7ca8c6ecedff6d07a02db164082ec37ae52d
comment:3 Changed 7 years ago by
- Component changed from statistics to symbolics
- Dependencies changed from #18223, #18586 to #18223, #18586, #18930
- Description modified (diff)
- Keywords asymptotics gsoc15 added
comment:4 Changed 7 years ago by
- Commit changed from 327d7ca8c6ecedff6d07a02db164082ec37ae52d to eb317b1495ea5844b59cda7bf9100d6d2b542200
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
96435ce | Merge branch 'asy/6.7/growthGroup' into asy/group-factory
|
b7d2273 | repair
|
655ec12 | rename to repr_short_to_parent
|
e60a325 | rewrite growth group factory
|
ea8ae66 | repr-option to suppress words "Growth Group"
|
b7f5c73 | rename repr-option-keyword and minor change in output
|
c0ec9fd | minor change in output of repr
|
f141775 | typos in module description fixed
|
62d824f | Merge branch 'asy/growthGroup' into asy/growthGroup-factory
|
eb317b1 | Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian;
|
comment:5 Changed 7 years ago by
- Commit changed from eb317b1495ea5844b59cda7bf9100d6d2b542200 to b18f319c196dc549444fd391fbab437603e6bb22
Branch pushed to git repo; I updated commit sha1. New commits:
164d21e | compare exponents directly instead with is_le_one
|
baa8f7c | implemented gens_monomial and adapted gen, gens, ngens
|
b477c62 | merge branch 'asy/growthGroup' into asy/growthGroup-factory and resolve a minor conflict
|
016d1f0 | merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian and resolve minor conflict
|
3749fa1 | refactored handling of generators
|
ba16c0f | Merge branch 'asy/growthGroup' into asy/growthGroup-factory
|
b18f319 | Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian
|
comment:6 Changed 7 years ago by
- Commit changed from b18f319c196dc549444fd391fbab437603e6bb22 to 83524299767ee9e123fe4dbed2f63945b154e8f7
Branch pushed to git repo; I updated commit sha1. New commits:
f4d16c2 | gens_monomial for cartesian products implemented
|
f33ba27 | fixed an issue with conversion of log(var)
|
c6d8584 | error with the element constructor fixed, doctest added
|
3564d23 | Merge branch 'asy/growthGroup' into asy/growthGroup-factory
|
dfbe937 | Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian
|
8352429 | element constructor of cartesian products simplified; helper method implemented
|
comment:7 Changed 7 years ago by
comment:8 Changed 7 years ago by
- Branch changed from u/behackl/asy/growth-group-cartesian to u/dkrenn/asy/growth-group-cartesian
comment:9 Changed 7 years ago by
- Commit changed from 83524299767ee9e123fe4dbed2f63945b154e8f7 to 62fc735ddb22e45ef8ef01e651c33ba062f17e56
comment:10 Changed 7 years ago by
- Commit changed from 62fc735ddb22e45ef8ef01e651c33ba062f17e56 to b7bb595cc37bab9e00165273de63a725590a684d
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
b634767 | change output of extract_variable_names
|
9c59fe1 | lazy import instead of import
|
3fe816a | extend extraction of variable names
|
9f65f8b | delete .one, since provided by categories
|
08f10df | inherite cartesian product from growth group as well
|
f67cb03 | simplify and adapt gens_monomial()
|
925d700 | pass flags of repr
|
68643c4 | better parsing variables
|
58dbda7 | better parsing growth group factory
|
b7bb595 | rewrite factory to use previous enhancements
|
comment:11 Changed 7 years ago by
- Commit changed from b7bb595cc37bab9e00165273de63a725590a684d to 8c8c1b5201001e527c9cae95eae5d87de40b5c6c
Branch pushed to git repo; I updated commit sha1. New commits:
8c8c1b5 | fix doctest coverage
|
comment:12 Changed 7 years ago by
- Status changed from new to needs_review
comment:13 Changed 7 years ago by
- Commit changed from 8c8c1b5201001e527c9cae95eae5d87de40b5c6c to ac80aeb7f0ca7efb3cbf0365c11ee16f4f53e548
Branch pushed to git repo; I updated commit sha1. New commits:
3a05be7 | Merge tag '6.9.beta5' into t/17600/asy/growthGroup
|
58f931d | add asymptotic_expansions index
|
9d6f2da | Merge branch 't/17600/asy/growthGroup' into t/18930/asy/growthGroup-factory
|
ca5da4a | Merge branch 't/18930/asy/growthGroup-factory' into t/18587/asy/growth-group-cartesian
|
ac80aeb | add growth_group_cartesian to index
|
comment:14 Changed 7 years ago by
Merge in 6.9.beta5
comment:15 Changed 7 years ago by
- Component changed from symbolics to asymptotic expansions
comment:16 Changed 7 years ago by
- Branch changed from u/dkrenn/asy/growth-group-cartesian to u/behackl/asy/growth-group-cartesian
- Commit changed from ac80aeb7f0ca7efb3cbf0365c11ee16f4f53e548 to 4a6174f7e86903243009162bd2e0b6bf6120c2b2
Merged latest positively reviewed dependencies (#17600, #18930) into this branch and resolved some small merge conflicts.
Last 10 new commits:
1c7a521 | corrected INPUT-section of parent_to_repr_short
|
6dee8b7 | abstract implementation --> basic implementation
|
3f88a9a | duplicate doctest removed
|
9489bad | element_constructor: input clarification; doctests marked as indirect
|
053ee33 | type(...) == ... --> isinstance(...)
|
7b49489 | clarification gens vs. gens_monomial
|
2bf6868 | prevent strange NotImplementedError from PowerSeriesRing
|
4f99031 | Merge branch 'asy/growthGroup' into asy/growthGroup-factory
|
55c78e2 | Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian
|
4a6174f | improved a short description
|
comment:17 Changed 7 years ago by
- Branch changed from u/behackl/asy/growth-group-cartesian to u/cheuberg/asy/growth-group-cartesian
comment:18 Changed 7 years ago by
- Commit changed from 4a6174f7e86903243009162bd2e0b6bf6120c2b2 to 6fefead5a7fd8cfe4e84d73641b4459c4671ceb4
- Milestone changed from sage-6.8 to sage-6.9
- Reviewers set to Clemens Heuberger
- Status changed from needs_review to needs_work
I reviewed this ticket, but without reviewing the dependency on #18223, so consider this as a conditional review.
I added a few reviewer commits. There is still a merge conflict with 6.9.rc0 caused by #17715.
Here are my other comments.
- Introduction
- I suggest to add a section heading before the note box (e.g. "Creation of Growth Groups") and to downgrade the note box to be an introductory paragraph of this section.
- Last sentence: "It is mathematically nonsense": explain how the order of factors of a single variable matters.
Variable.__init__
:ValueError
"is not a valid name": not testedVariable.extract_variable_names
:- I am surprised that a more or less complete symbolic expression parser is needed here. Would it not be simpler to use
SR(s)
and then call.variables()
, possibly on subexpressions in order to detect duplicates? It seems thatSR(s)
would create the symbolic variables on the fly, but does not inject them into the global name space, so this seems to be what is needed here. - I do not think the following is intentional:
sage: Variable.extract_variable_names('a + (b + c) + d') ('b', 'c', 'd')
- Why does the regular expression for number contain a
$
but no^
?
- I am surprised that a more or less complete symbolic expression parser is needed here. Would it not be simpler to use
GenericGrowthGroup.gens_monomial
: Replace "is only implemented for" by "must be implemented in"MonomialGrowthGroup
: Is the following intentionally allowed:sage: from sage.rings.asymptotic.growth_group import MonomialGrowthGroup sage: MonomialGrowthGroup(ZZ, 'x+y') Growth Group x+y^ZZ
MonomialGrowthGroup._convert_
:- Is the following intentional:
sage: import sage.rings.asymptotic.growth_group as agg sage: P = agg.MonomialGrowthGroup(ZZ, 'x') sage: P(1) 1 sage: P('x^2') x^2 sage: P('1') Traceback (most recent call last): ... ValueError: Cannot convert 1. sage: G('x^)42(') x^42
- As in 3a, couldn't the string parsing be done by the symbolic ring?
- Is the following intentional:
GrowthGroupFactory.create_key_and_extra_argument
: Again, my suggestion is to delegate the string parsing to the symbolic ring. Note thatSR('x^ZZ')
happily acceptsZZ
as a variable, so I imagine that postprocessing here would not involve big problems. I do not read this string parser here before the decision on its eventual fate.- Add one or more links from
growth_group.py
togrowth_group_cartesian.py
. CartesianProductFactory
:- indicate (somewhere, probably at a more prominent place) what order is chosen automatically (apparently, lexicographic if the variables are equal, component-wise otherwise). That would also explain why order does matter (cf. 1b).
- the example with "do not have disjoint variables" is somewhat puzzeling, because the cartesian product of
A
andB
was allowed. - (continuation of b while reading the code): apparently, sets of variables of the factors must be equal or disjoint.
- mark doctests as indirect
GenericProduct
: mark doctests as indirect.- Check links
~sage.sets.cartesian_product.CartesianProductPosets
andsage.sets.cartesian_product.CartesianProductPosets.Element
(do not work for me, possibly time intensive recreation of documentation would help). GenericProduct._coerce_map_from_
: don't the factors coerce into the product?sage: from sage.rings.asymptotic.growth_group import GrowthGroup sage: G = GrowthGroup('x^ZZ') sage: H = GrowthGroup('log(x)^ZZ') sage: K = cartesian_product([G, H]) sage: K.has_coerce_map_from(G) False sage: K.has_coerce_map_from(H) False
- Expand documentation of
UnivariateProduct
andMultivariateProduct
: at least, include a link toGenericProduct
and explain the choice of order.
New commits:
59430d8 | Trac #18587: remove redundant doctest
|
f11377d | Trac #18587: minor language issues
|
894e3a8 | Trac #18587: Fix ReSt errors
|
61085d0 | Trac #18587: add heading "Classes and Methods"
|
6fefead | Trac #18587: Fix minor errors
|
comment:19 Changed 7 years ago by
- Branch changed from u/cheuberg/asy/growth-group-cartesian to u/behackl/asy/growth-group-cartesian
- Commit changed from 6fefead5a7fd8cfe4e84d73641b4459c4671ceb4 to f7e917fa07fbe47a730f9cdec4a6eb78a9b094f8
Merged 6.9.rc0
into this branch.
Last 10 new commits:
ca5da4a | Merge branch 't/18930/asy/growthGroup-factory' into t/18587/asy/growth-group-cartesian
|
ac80aeb | add growth_group_cartesian to index
|
55c78e2 | Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian
|
4a6174f | improved a short description
|
59430d8 | Trac #18587: remove redundant doctest
|
f11377d | Trac #18587: minor language issues
|
894e3a8 | Trac #18587: Fix ReSt errors
|
61085d0 | Trac #18587: add heading "Classes and Methods"
|
6fefead | Trac #18587: Fix minor errors
|
f7e917f | Merge tag '6.9.rc0' into asy/growth-group-cartesian
|
comment:20 Changed 7 years ago by
- Commit changed from f7e917fa07fbe47a730f9cdec4a6eb78a9b094f8 to f88d3ead2feadd76bf3fccbd6488f7d0c36c501d
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
d34fa05 | Merge branch 'cat/cartesian-product-posets' into asy/growth-group-cartesian
|
9fcf771 | extended introduction (comment 1)
|
125c0c4 | fix imports and take care of changes in #18223
|
dc2504d | cartesian_product doctests: indirect (comments 9/10)
|
665f2ba | improve documentation (CartesianProductFactory,
|
26eb89c | remove unreachable ValueError (comment 2)
|
0887284 | extract_variable_names: let SR parse (comment 3)
|
d35c406 | fix doctest
|
9528dfe | language (comment 4)
|
f88d3ea | MonomialGrowthGroup._convert_: let SR parse string + take care of '1' (comment 6)
|
comment:21 follow-up: ↓ 22 Changed 7 years ago by
Replying to cheuberg:
I reviewed this ticket, but without reviewing the dependency on #18223, so consider this as a conditional review.
I added a few reviewer commits. There is still a merge conflict with 6.9.rc0 caused by #17715.
Hello Clemens! Thank you for your review!
The merge conflict with 6.9.rc0
has been resolved.
Here are my other comments.
- Introduction
- I suggest to add a section heading before the note box (e.g. "Creation of Growth Groups") and to downgrade the note box to be an introductory paragraph of this section.
- Last sentence: "It is mathematically nonsense": explain how the order of factors of a single variable matters.
Done and done.
Variable.__init__
:ValueError
"is not a valid name": not tested
Removed this particular piece of code; the check is not necessary: the parser only allows strings that are valid identifiers (see my answer to 3.).
Variable.extract_variable_names
:
- I am surprised that a more or less complete symbolic expression parser is needed here. Would it not be simpler to use
SR(s)
and then call.variables()
, possibly on subexpressions in order to detect duplicates? It seems thatSR(s)
would create the symbolic variables on the fly, but does not inject them into the global name space, so this seems to be what is needed here.
Changed this parser to the symbolic ring.
- I do not think the following is intentional:
sage: Variable.extract_variable_names('a + (b + c) + d') ('b', 'c', 'd')
Indeed, this was not intentional. With the symbolic ring as the variable parser, this does not happen. I've added this as a doctest.
- Why does the regular expression for number contain a
$
but no^
?
This is now irrelevant.
GenericGrowthGroup.gens_monomial
: Replace "is only implemented for" by "must be implemented in"
Done.
MonomialGrowthGroup
: Is the following intentionally allowed:sage: from sage.rings.asymptotic.growth_group import MonomialGrowthGroup sage: MonomialGrowthGroup(ZZ, 'x+y') Growth Group x+y^ZZ
Yes, this is intentional. On this ticket, the representation is ugly, but this is fixed in #19083.
MonomialGrowthGroup._convert_
:
- Is the following intentional:
sage: import sage.rings.asymptotic.growth_group as agg sage: P = agg.MonomialGrowthGroup(ZZ, 'x') sage: P(1) 1 sage: P('x^2') x^2 sage: P('1') Traceback (most recent call last): ... ValueError: Cannot convert 1. sage: G('x^)42(') x^42
The first two statements work as intended. The others are fixed here.
- As in 3a, couldn't the string parsing be done by the symbolic ring?
Yes. Done.
GrowthGroupFactory.create_key_and_extra_argument
: Again, my suggestion is to delegate the string parsing to the symbolic ring. Note thatSR('x^ZZ')
happily acceptsZZ
as a variable, so I imagine that postprocessing here would not involve big problems. I do not read this string parser here before the decision on its eventual fate.
Unfortunately, I think that this should not be parsed by the symbolic ring: the cartesian product which we model with *
is not commutative: if the same variable is used, the growth groups are ordered lexicographically. By using the symbolic ring, the input order might be destroyed/ignored.
- Add one or more links from
growth_group.py
togrowth_group_cartesian.py
.
We took extensive care with respect to the documentation on #19083. There, some links have been added.
CartesianProductFactory
:
- indicate (somewhere, probably at a more prominent place) what order is chosen automatically (apparently, lexicographic if the variables are equal, component-wise otherwise). That would also explain why order does matter (cf. 1b).
I've added a note-block to the docstring of the class.
- the example with "do not have disjoint variables" is somewhat puzzeling, because the cartesian product of
A
andB
was allowed.
I understand the confusion. However, I think that the error message is quite helpful in this case:
sage: cartesian_product([A, E]); G Traceback (most recent call last): ... ValueError: Growth groups (Growth Group x^ZZ, Growth Group x^ZZ * y^ZZ) do not have pairwise disjoint variables.
Mainly, this is because the cartesian product cannot decide which order to use in this case. Also, I think that we should restrict building cartesian products of growth groups such that a factor may only occur once (currently, x^ZZ * x^ZZ
is possible). I'll discuss this with Daniel.
- (continuation of b while reading the code): apparently, sets of variables of the factors must be equal or disjoint.
I've added this remark to the note-block in the docstring of the factory.
- mark doctests as indirect
Done.
GenericProduct
: mark doctests as indirect.
Done.
- Check links
~sage.sets.cartesian_product.CartesianProductPosets
andsage.sets.cartesian_product.CartesianProductPosets.Element
(do not work for me, possibly time intensive recreation of documentation would help).
I corrected the links (we shifted some code in #18223), they work for me now -- however, that should not have affected you. (Sometimes, the documentation really wants to be built from scratch. :-)
)
GenericProduct._coerce_map_from_
: don't the factors coerce into the product?sage: from sage.rings.asymptotic.growth_group import GrowthGroup sage: G = GrowthGroup('x^ZZ') sage: H = GrowthGroup('log(x)^ZZ') sage: K = cartesian_product([G, H]) sage: K.has_coerce_map_from(G) False sage: K.has_coerce_map_from(H) False
No, not in general. However, due to our special structure (and as soon as we actually disallow copies of the same growth group in a cartesian product) this can be implemented. This should be handled in our coercion ticket, #19073.
- Expand documentation of
UnivariateProduct
andMultivariateProduct
: at least, include a link toGenericProduct
and explain the choice of order.
Done.
comment:22 in reply to: ↑ 21 ; follow-up: ↓ 25 Changed 7 years ago by
Replying to behackl:
Replying to cheuberg:
Variable.__init__
:ValueError
"is not a valid name": not testedRemoved this particular piece of code; the check is not necessary: the parser only allows strings that are valid identifiers (see my answer to 3.).
I do not think so: extract_variable_names
is only called when repr is None
, the removed code was only called when repr is not None
. Indeed, the following does now work:
sage: from sage.rings.asymptotic.growth_group import Variable sage: Variable("(:-)", repr="icecream").var_bases ('(:-)',)
GrowthGroupFactory.create_key_and_extra_argument
: Again, my suggestion is to delegate the string parsing to the symbolic ring. Note thatSR('x^ZZ')
happily acceptsZZ
as a variable, so I imagine that postprocessing here would not involve big problems. I do not read this string parser here before the decision on its eventual fate.Unfortunately, I think that this should not be parsed by the symbolic ring: the cartesian product which we model with
*
is not commutative: if the same variable is used, the growth groups are ordered lexicographically. By using the symbolic ring, the input order might be destroyed/ignored.
I see.
The current code makes some effort to recognize **
in create_key_and_extra_args
, but **
is not recognized in create_object
.
CartesianProductFactory
:
- the example with "do not have disjoint variables" is somewhat puzzeling, because the cartesian product of
A
andB
was allowed.I understand the confusion. However, I think that the error message is quite helpful in this case:
sage: cartesian_product([A, E]); G Traceback (most recent call last): ... ValueError: Growth groups (Growth Group x^ZZ, Growth Group x^ZZ * y^ZZ) do not have pairwise disjoint variables.Mainly, this is because the cartesian product cannot decide which order to use in this case.
perhaps rephrase error message along the lines of "factors must have equal or disjoint variables"?
Also, I think that we should restrict building cartesian products of growth groups such that a factor may only occur once (currently,
x^ZZ * x^ZZ
is possible). I'll discuss this with Daniel.
that might be hard.
comment:23 Changed 7 years ago by
- Branch changed from u/behackl/asy/growth-group-cartesian to u/cheuberg/asy/growth-group-cartesian
comment:24 Changed 7 years ago by
- Commit changed from f88d3ead2feadd76bf3fccbd6488f7d0c36c501d to 6d3e4f4ce27b8b0cb252f05a352458f7075e0b1a
comment:25 in reply to: ↑ 22 ; follow-up: ↓ 26 Changed 7 years ago by
- Branch changed from u/cheuberg/asy/growth-group-cartesian to u/behackl/asy/growth-group-cartesian
- Commit changed from 6d3e4f4ce27b8b0cb252f05a352458f7075e0b1a to 7f209ea7928606090e597d0633685649739932b8
- Status changed from needs_work to needs_review
Replying to cheuberg:
Replying to behackl:
Replying to cheuberg:
Variable.__init__
:ValueError
"is not a valid name": not testedRemoved this particular piece of code; the check is not necessary: the parser only allows strings that are valid identifiers (see my answer to 3.).
I do not think so:
extract_variable_names
is only called whenrepr is None
, the removed code was only called whenrepr is not None
. Indeed, the following does now work:sage: from sage.rings.asymptotic.growth_group import Variable sage: Variable("(:-)", repr="icecream").var_bases ('(:-)',)
Mea culpa, I misread the code. I've reverted the commit.
GrowthGroupFactory.create_key_and_extra_argument
: Again, my suggestion is to delegate the string parsing to the symbolic ring. Note thatSR('x^ZZ')
happily acceptsZZ
as a variable, so I imagine that postprocessing here would not involve big problems. I do not read this string parser here before the decision on its eventual fate.Unfortunately, I think that this should not be parsed by the symbolic ring: the cartesian product which we model with
*
is not commutative: if the same variable is used, the growth groups are ordered lexicographically. By using the symbolic ring, the input order might be destroyed/ignored.I see.
The current code makes some effort to recognize
**
increate_key_and_extra_args
, but**
is not recognized increate_object
.
Minor discrepancies like this are fixed on the cleanup ticket #19083. No need to introduce additional merge conflicts here.
CartesianProductFactory
:
- the example with "do not have disjoint variables" is somewhat puzzeling, because the cartesian product of
A
andB
was allowed.I understand the confusion. However, I think that the error message is quite helpful in this case:
sage: cartesian_product([A, E]); G Traceback (most recent call last): ... ValueError: Growth groups (Growth Group x^ZZ, Growth Group x^ZZ * y^ZZ) do not have pairwise disjoint variables.Mainly, this is because the cartesian product cannot decide which order to use in this case.
perhaps rephrase error message along the lines of "factors must have equal or disjoint variables"?
Done; improved the error message.
Also, I think that we should restrict building cartesian products of growth groups such that a factor may only occur once (currently,
x^ZZ * x^ZZ
is possible). I'll discuss this with Daniel.that might be hard.
Turns out that this is very true. We will start with disallowing certain products like x^ZZ * x^ZZ
and x^ZZ * x^QQ
in the cleanup ticket. Altering the factory here only produces more merge conflicts with #19083 (the entire factory is rewritten there, meaning that the code would have to be written twice).
New commits:
66759bb | Revert "remove unreachable ValueError (comment 2)"
|
0642564 | doctest added
|
7f209ea | improved error message (equal or disjoint var.)
|
comment:26 in reply to: ↑ 25 ; follow-up: ↓ 28 Changed 7 years ago by
Replying to behackl:
Replying to cheuberg:
The current code makes some effort to recognize
**
increate_key_and_extra_args
, but**
is not recognized increate_object
.Minor discrepancies like this are fixed on the cleanup ticket #19083. No need to introduce additional merge conflicts here.
ok.
Also, I think that we should restrict building cartesian products of growth groups such that a factor may only occur once (currently,
x^ZZ * x^ZZ
is possible). I'll discuss this with Daniel.that might be hard.
Turns out that this is very true. We will start with disallowing certain products like
x^ZZ * x^ZZ
andx^ZZ * x^QQ
in the cleanup ticket. Altering the factory here only produces more merge conflicts with #19083 (the entire factory is rewritten there, meaning that the code would have to be written twice).
ok.
This is a conditional positive review from my side: conditional on the dependency #18223.
comment:27 Changed 7 years ago by
- Branch changed from u/behackl/asy/growth-group-cartesian to u/dkrenn/asy/growth-group-cartesian
comment:28 in reply to: ↑ 26 ; follow-up: ↓ 29 Changed 7 years ago by
- Commit changed from 7f209ea7928606090e597d0633685649739932b8 to 74547e6aac076261a6f7da6577d771eb9a830e60
Replying to cheuberg:
This is a conditional positive review from my side: conditional on the dependency #18223.
Merged in #18223 and adapted code. Please cross-review...and set it to positive ;)
New commits:
70aa9c4 | rename CartesianProductPosets to CartesianProductPoset
|
f21990d | code-simplify CartesianProduct assignment
|
8f9a619 | add a doctest dealing with coercion while comparing
|
3b923b7 | Merge branch 'u/dkrenn/cat/cartesian-product-posets' of trac.sagemath.org:sage into t/18587/asy/growth-group-cartesian
|
74547e6 | fix code after changes by previous merge
|
comment:29 in reply to: ↑ 28 Changed 7 years ago by
- Status changed from needs_review to positive_review
comment:30 Changed 7 years ago by
This ticket depends on a "duplicate" ticket. It will not be merged until #18586 is merged, i.e. most likely never.
comment:31 Changed 7 years ago by
- Dependencies changed from #18223, #18586, #18930 to #18223, #18930
comment:32 Changed 7 years ago by
- Branch changed from u/dkrenn/asy/growth-group-cartesian to 74547e6aac076261a6f7da6577d771eb9a830e60
- Resolution set to fixed
- Status changed from positive_review to closed
Last 10 new commits:
Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian
GrowthGroupFactory extension: construction of cartesian products
header modified
no unneccessary cartesian products
representation of cartesian product elements improved
module description improved
element constructor of monomial growth groups also try to parse the
element constructor for cartesian product elements implemented
problem with parsing of strings fixed
module documentation: added some doctests