Opened 4 years ago

Closed 4 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) Commit: 74547e6aac076261a6f7da6577d771eb9a830e60
Dependencies: #18223, #18930 Stopgaps:

Description (last modified by behackl)

This implements cartesian products (via #18223) of growth groups (see #17600; or meta ticket #17601).

In particular, the factory for growth groups introduced in #18930 is extended such that cartesian products can also be generated.

Change History (32)

comment:1 Changed 4 years ago by dkrenn

  • Cc behackl cheuberg added
  • Type changed from PLEASE CHANGE to enhancement

comment:2 Changed 4 years ago by behackl

  • Branch changed from u/dkrenn/asy/growth-group-cartesian to u/behackl/asy/growth-group-cartesian
  • Commit changed from c35bd864269ad55baac18c649741fa445aa6cf4f to 327d7ca8c6ecedff6d07a02db164082ec37ae52d

Last 10 new commits:

5b12589Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian
0532942GrowthGroupFactory extension: construction of cartesian products
c547712header modified
3af46e7no unneccessary cartesian products
7e0fb9erepresentation of cartesian product elements improved
ca73a61module description improved
ab9bcafelement constructor of monomial growth groups also try to parse the
a899bb7element constructor for cartesian product elements implemented
537f22cproblem with parsing of strings fixed
327d7camodule documentation: added some doctests

comment:3 Changed 4 years ago by behackl

  • Authors changed from Daniel Krenn to Daniel Krenn, Benjamin Hackl
  • 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 4 years ago by git

  • Commit changed from 327d7ca8c6ecedff6d07a02db164082ec37ae52d to eb317b1495ea5844b59cda7bf9100d6d2b542200

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

96435ceMerge branch 'asy/6.7/growthGroup' into asy/group-factory
b7d2273repair
655ec12rename to repr_short_to_parent
e60a325rewrite growth group factory
ea8ae66repr-option to suppress words "Growth Group"
b7f5c73rename repr-option-keyword and minor change in output
c0ec9fdminor change in output of repr
f141775typos in module description fixed
62d824fMerge branch 'asy/growthGroup' into asy/growthGroup-factory
eb317b1Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian;

comment:5 Changed 4 years ago by git

  • Commit changed from eb317b1495ea5844b59cda7bf9100d6d2b542200 to b18f319c196dc549444fd391fbab437603e6bb22

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

164d21ecompare exponents directly instead with is_le_one
baa8f7cimplemented gens_monomial and adapted gen, gens, ngens
b477c62merge branch 'asy/growthGroup' into asy/growthGroup-factory and resolve a minor conflict
016d1f0merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian and resolve minor conflict
3749fa1refactored handling of generators
ba16c0fMerge branch 'asy/growthGroup' into asy/growthGroup-factory
b18f319Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian

comment:6 Changed 4 years ago by git

  • Commit changed from b18f319c196dc549444fd391fbab437603e6bb22 to 83524299767ee9e123fe4dbed2f63945b154e8f7

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

f4d16c2gens_monomial for cartesian products implemented
f33ba27fixed an issue with conversion of log(var)
c6d8584error with the element constructor fixed, doctest added
3564d23Merge branch 'asy/growthGroup' into asy/growthGroup-factory
dfbe937Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian
8352429element constructor of cartesian products simplified; helper method implemented

comment:7 Changed 4 years ago by dkrenn

  • Authors changed from Daniel Krenn, Benjamin Hackl to Benjamin Hackl, Daniel Krenn

comment:8 Changed 4 years ago by dkrenn

  • Branch changed from u/behackl/asy/growth-group-cartesian to u/dkrenn/asy/growth-group-cartesian

comment:9 Changed 4 years ago by git

  • Commit changed from 83524299767ee9e123fe4dbed2f63945b154e8f7 to 62fc735ddb22e45ef8ef01e651c33ba062f17e56

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

1815847two minor modifications of docstrings
6e33615modify to to make things running...
62fc735write docstings of cartesian factory

comment:10 Changed 4 years ago by git

  • Commit changed from 62fc735ddb22e45ef8ef01e651c33ba062f17e56 to b7bb595cc37bab9e00165273de63a725590a684d

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

b634767change output of extract_variable_names
9c59fe1lazy import instead of import
3fe816aextend extraction of variable names
9f65f8bdelete .one, since provided by categories
08f10dfinherite cartesian product from growth group as well
f67cb03simplify and adapt gens_monomial()
925d700pass flags of repr
68643c4better parsing variables
58dbda7better parsing growth group factory
b7bb595rewrite factory to use previous enhancements

comment:11 Changed 4 years ago by git

  • Commit changed from b7bb595cc37bab9e00165273de63a725590a684d to 8c8c1b5201001e527c9cae95eae5d87de40b5c6c

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

8c8c1b5fix doctest coverage

comment:12 Changed 4 years ago by dkrenn

  • Status changed from new to needs_review

comment:13 Changed 4 years ago by git

  • Commit changed from 8c8c1b5201001e527c9cae95eae5d87de40b5c6c to ac80aeb7f0ca7efb3cbf0365c11ee16f4f53e548

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

3a05be7Merge tag '6.9.beta5' into t/17600/asy/growthGroup
58f931dadd asymptotic_expansions index
9d6f2daMerge branch 't/17600/asy/growthGroup' into t/18930/asy/growthGroup-factory
ca5da4aMerge branch 't/18930/asy/growthGroup-factory' into t/18587/asy/growth-group-cartesian
ac80aebadd growth_group_cartesian to index

comment:14 Changed 4 years ago by dkrenn

Merge in 6.9.beta5

comment:15 Changed 4 years ago by dkrenn

  • Component changed from symbolics to asymptotic expansions

comment:16 Changed 4 years ago by behackl

  • 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:

1c7a521corrected INPUT-section of parent_to_repr_short
6dee8b7abstract implementation --> basic implementation
3f88a9aduplicate doctest removed
9489badelement_constructor: input clarification; doctests marked as indirect
053ee33type(...) == ... --> isinstance(...)
7b49489clarification gens vs. gens_monomial
2bf6868prevent strange NotImplementedError from PowerSeriesRing
4f99031Merge branch 'asy/growthGroup' into asy/growthGroup-factory
55c78e2Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian
4a6174fimproved a short description

comment:17 Changed 4 years ago by cheuberg

  • Branch changed from u/behackl/asy/growth-group-cartesian to u/cheuberg/asy/growth-group-cartesian

comment:18 Changed 4 years ago by cheuberg

  • 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.

  1. 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.
  2. Variable.__init__: ValueError "is not a valid name": not tested
  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 that SR(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 ^?
  4. GenericGrowthGroup.gens_monomial: Replace "is only implemented for" by "must be implemented in"
  5. 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
    
  6. 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?
  7. GrowthGroupFactory.create_key_and_extra_argument: Again, my suggestion is to delegate the string parsing to the symbolic ring. Note that SR('x^ZZ') happily accepts ZZ 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.
  8. Add one or more links from growth_group.py to growth_group_cartesian.py.
  9. 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 and B 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
  10. GenericProduct: mark doctests as indirect.
  11. Check links ~sage.sets.cartesian_product.CartesianProductPosets and sage.sets.cartesian_product.CartesianProductPosets.Element (do not work for me, possibly time intensive recreation of documentation would help).
  12. 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
    
  13. Expand documentation of UnivariateProduct and MultivariateProduct: at least, include a link to GenericProduct and explain the choice of order.

New commits:

59430d8Trac #18587: remove redundant doctest
f11377dTrac #18587: minor language issues
894e3a8Trac #18587: Fix ReSt errors
61085d0Trac #18587: add heading "Classes and Methods"
6fefeadTrac #18587: Fix minor errors

comment:19 Changed 4 years ago by behackl

  • 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:

ca5da4aMerge branch 't/18930/asy/growthGroup-factory' into t/18587/asy/growth-group-cartesian
ac80aebadd growth_group_cartesian to index
55c78e2Merge branch 'asy/growthGroup-factory' into asy/growth-group-cartesian
4a6174fimproved a short description
59430d8Trac #18587: remove redundant doctest
f11377dTrac #18587: minor language issues
894e3a8Trac #18587: Fix ReSt errors
61085d0Trac #18587: add heading "Classes and Methods"
6fefeadTrac #18587: Fix minor errors
f7e917fMerge tag '6.9.rc0' into asy/growth-group-cartesian

comment:20 Changed 4 years ago by git

  • Commit changed from f7e917fa07fbe47a730f9cdec4a6eb78a9b094f8 to f88d3ead2feadd76bf3fccbd6488f7d0c36c501d

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

d34fa05Merge branch 'cat/cartesian-product-posets' into asy/growth-group-cartesian
9fcf771extended introduction (comment 1)
125c0c4fix imports and take care of changes in #18223
dc2504dcartesian_product doctests: indirect (comments 9/10)
665f2baimprove documentation (CartesianProductFactory,
26eb89cremove unreachable ValueError (comment 2)
0887284extract_variable_names: let SR parse (comment 3)
d35c406fix doctest
9528dfelanguage (comment 4)
f88d3eaMonomialGrowthGroup._convert_: let SR parse string + take care of '1' (comment 6)

comment:21 follow-up: Changed 4 years ago by behackl

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.

  1. 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.

  1. 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.).

  1. 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 that SR(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.

  1. GenericGrowthGroup.gens_monomial: Replace "is only implemented for" by "must be implemented in"

Done.

  1. 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.

  1. 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.

  1. GrowthGroupFactory.create_key_and_extra_argument: Again, my suggestion is to delegate the string parsing to the symbolic ring. Note that SR('x^ZZ') happily accepts ZZ 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.

  1. Add one or more links from growth_group.py to growth_group_cartesian.py.

We took extensive care with respect to the documentation on #19083. There, some links have been added.

  1. 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 and B 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.

  1. GenericProduct: mark doctests as indirect.

Done.

  1. Check links ~sage.sets.cartesian_product.CartesianProductPosets and sage.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. :-))

  1. 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.

  1. Expand documentation of UnivariateProduct and MultivariateProduct: at least, include a link to GenericProduct and explain the choice of order.

Done.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 4 years ago by cheuberg

Replying to behackl:

Replying to cheuberg:

  1. 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.).

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
('(:-)',)
  1. GrowthGroupFactory.create_key_and_extra_argument: Again, my suggestion is to delegate the string parsing to the symbolic ring. Note that SR('x^ZZ') happily accepts ZZ 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.

  1. CartesianProductFactory:
    • the example with "do not have disjoint variables" is somewhat puzzeling, because the cartesian product of A and B 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 4 years ago by cheuberg

  • Branch changed from u/behackl/asy/growth-group-cartesian to u/cheuberg/asy/growth-group-cartesian

comment:24 Changed 4 years ago by git

  • Commit changed from f88d3ead2feadd76bf3fccbd6488f7d0c36c501d to 6d3e4f4ce27b8b0cb252f05a352458f7075e0b1a

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

dea3c96Trac #18587: additional doctest
6d3e4f4Trac #18587: nicer output of one link target

comment:25 in reply to: ↑ 22 ; follow-up: Changed 4 years ago by behackl

  • 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:

  1. 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.).

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
('(:-)',)

Mea culpa, I misread the code. I've reverted the commit.

  1. GrowthGroupFactory.create_key_and_extra_argument: Again, my suggestion is to delegate the string parsing to the symbolic ring. Note that SR('x^ZZ') happily accepts ZZ 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.

Minor discrepancies like this are fixed on the cleanup ticket #19083. No need to introduce additional merge conflicts here.

  1. CartesianProductFactory:
    • the example with "do not have disjoint variables" is somewhat puzzeling, because the cartesian product of A and B 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:

66759bbRevert "remove unreachable ValueError (comment 2)"
0642564doctest added
7f209eaimproved error message (equal or disjoint var.)

comment:26 in reply to: ↑ 25 ; follow-up: Changed 4 years ago by cheuberg

Replying to behackl:

Replying to cheuberg:

The current code makes some effort to recognize ** in create_key_and_extra_args, but ** is not recognized in create_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 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).

ok.

This is a conditional positive review from my side: conditional on the dependency #18223.

comment:27 Changed 4 years ago by dkrenn

  • Branch changed from u/behackl/asy/growth-group-cartesian to u/dkrenn/asy/growth-group-cartesian

comment:28 in reply to: ↑ 26 ; follow-up: Changed 4 years ago by dkrenn

  • 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:

70aa9c4rename CartesianProductPosets to CartesianProductPoset
f21990dcode-simplify CartesianProduct assignment
8f9a619add a doctest dealing with coercion while comparing
3b923b7Merge branch 'u/dkrenn/cat/cartesian-product-posets' of trac.sagemath.org:sage into t/18587/asy/growth-group-cartesian
74547e6fix code after changes by previous merge

comment:29 in reply to: ↑ 28 Changed 4 years ago by cheuberg

  • Status changed from needs_review to positive_review

Replying to dkrenn:

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 ;)

Done.

comment:30 Changed 4 years ago by vbraun

This ticket depends on a "duplicate" ticket. It will not be merged until #18586 is merged, i.e. most likely never.

comment:31 Changed 4 years ago by cheuberg

  • Dependencies changed from #18223, #18586, #18930 to #18223, #18930

comment:32 Changed 4 years ago by vbraun

  • Branch changed from u/dkrenn/asy/growth-group-cartesian to 74547e6aac076261a6f7da6577d771eb9a830e60
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.