#16261 closed enhancement (fixed)
Default behaviour of AdditiveAbelianGroup(a_tuple)
Reported by:  Nathann Cohen  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  group theory  Keywords:  
Cc:  Volker Braun, John Palmieri, Simon King, Travis Scrimshaw  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  36cf500 (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
This ticket follows the conversation on https://groups.google.com/forum/#!topic/sagesupport/yexpjig9BSg
sage: H = AdditiveAbelianGroup([0,2]) sage: H((2,0)) (0, 0) sage: H(vector((2,0))) (2, 0) sage: H((1,0)).order() 2 sage: H(vector((1,0))).order() +Infinity
With this branch there is no need to wrap a tuple into a vector to avoid the "if isinstance(x,(tuple,list))" and what should be the default behaviour IS the default behaviour. And it barely breaks doctests once one replaced the old [G([1,2])
by G.linear_combination_of_smith_form_gens([1,2])
Attachments (1)
Change History (40)
comment:1 Changed 8 years ago by
Branch:  → u/ncohen/16261 

Commit:  → 270545adf1dfc9e7554cb2f78700402f787a56b4 
comment:2 Changed 8 years ago by
Commit:  270545adf1dfc9e7554cb2f78700402f787a56b4 → cdecd43022a6c78f40655374c3eb804b810483e9 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
cdecd43  trac #16261: Default behaviour of AdditiveAbelianGroup(a_tuple)

comment:3 Changed 8 years ago by
Status:  new → needs_review 

comment:4 Changed 8 years ago by
Status:  needs_review → needs_work 

You didn't implement linear_combination_of_gens
, leaving no way to do that without tripping the warning. Also, you should obviously run the whole testsuite.
comment:5 Changed 8 years ago by
I do not think it is bad to always generate this warning, and I find linear_combination_of_gens
far too extreme. I am pretty sure that nobody who really uses this only had inputs in smith form, or he would have given up already.
I am running all the tests right now (it found something in ellipti curves already) but it takes times.. And well, we used to have patchbots for this :P
Nathann
comment:6 Changed 8 years ago by
by the way : anybody who used the syntax on purpose will move to .linear_sum_of_smith_generators
, and those who wanted the normal sum wrapped everything with "vectors" and will not even see the warning :P
comment:7 Changed 8 years ago by
Commit:  cdecd43022a6c78f40655374c3eb804b810483e9 → 04683ed9d981b1a561163867eee8f63044733fc9 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
04683ed  trac #16261: Default behaviour of AdditiveAbelianGroup(a_tuple)

comment:8 Changed 8 years ago by
Now passes all tests except one, see discussion on the sagesupport thread.
Nathann
comment:9 Changed 8 years ago by
Commit:  04683ed9d981b1a561163867eee8f63044733fc9 → fe241f151b7652552560d0867803309bc3e2f8e0 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
fe241f1  trac #16261: Default behaviour of AdditiveAbelianGroup(a_tuple)

comment:11 Changed 8 years ago by
Commit:  fe241f151b7652552560d0867803309bc3e2f8e0 → 5d78cfc86f64819c0427941c882015dd2e3df73e 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5d78cfc  trac #16261: Default behaviour of AdditiveAbelianGroup(a_tuple)

comment:12 Changed 8 years ago by
Status:  needs_work → needs_review 

comment:13 followup: 14 Changed 8 years ago by
The problem is not quite introduced by this change, but this change does make the problem a little more visible: Why are you singling out Z/2Z x Z/3Z
as a group that can be explicitly represented and not
Z^2/ span( [1,1],[1,5])
, with generators the classes of the standard basis?
comment:14 followup: 15 Changed 8 years ago by
Yooooooooo !
The problem is not quite introduced by this change, but this change does make the problem a little more visible: Why are you singling out
Z/2Z x Z/3Z
as a group that can be explicitly represented and notZ^2/ span( [1,1],[1,5])
, with generators the classes of the standard basis?
Do I misunderstand your question or are you saying that you would like to use any base you like instead of (1,0),(0,1), or the smith form ?
I just need this new form for a couple of things I am implementing... But I am afraid of having to touch these files myself, because I have a good idea of what I forgot about groups since I began a PhD in graph theory... and it scares me :D
Nathann
comment:15 followup: 16 Changed 8 years ago by
Replying to ncohen:
Do I misunderstand your question or are you saying that you would like to use any base you like instead of (1,0),(0,1), or the smith form ?
Basically, yes. The problem is that presently, AdditiveAbelianGroup? is a little too lax in what it accepts as "invariants". In the classification of (finite) abelian groups, the "invariants" [a1,a2,...,ar]
are subject to the additional condition that ai
divides a(i+1)
. Otherwise they aren't really invariants.
If you're going to allow abelian groups to be specified in forms different from smith normal form, I think it's a little strange to just limit to these "diagonal" forms. In general, you specify a finitely generated abelian group using generators and relations, i.e., ZZ^r
modulo some submodule. Since the underlying machinery is that of ZZmodules anyway, why not do it properly?
comment:16 Changed 8 years ago by
Yoooooooooooo !
If you're going to allow abelian groups to be specified in forms different from smith normal form, I think it's a little strange to just limit to these "diagonal" forms. In general, you specify a finitely generated abelian group using generators and relations, i.e.,
ZZ^r
modulo some submodule. Since the underlying machinery is that of ZZmodules anyway, why not do it properly?
Ahahaah. Very simple : I have no idea how. I already said that I didn't feel at ease editing group code, so if you want anything more general than what is in the branch, I'm sorry to say that you will have to write the code yourself :)
And I agree with you that more bases should be allowed indeed.
Oh, and if you have any idea of how to make Sage see G = GF(8,'x').cartesian_product(GF(5))
as a group, it would be cool by the way.
sage: G([1,1])+G([1,1]) TypeError: unsupported operand type(s) for +: 'CartesianProduct_with_category.element_class' and 'CartesianProduct_with_category.element_class'
God.. These things are SO broken :
sage: list(G) AttributeError: 'CartesianProduct_with_category' object has no attribute 'list'
Perhaps this is the problem :
sage: g=GF(8,'x') sage: g.category() Category of finite fields sage: gg=g.cartesian_product(g) sage: gg.category() Category of Cartesian products of monoids
This gg
should be a product of groups I guess...
Nathann
comment:17 Changed 8 years ago by
Milestone:  sage6.2 → sage6.3 

comment:18 Changed 8 years ago by
Work issues:  → merge conflict 

comment:19 Changed 8 years ago by
Commit:  5d78cfc86f64819c0427941c882015dd2e3df73e → c1858741b33741bac325cf96a8166712273151bf 

Branch pushed to git repo; I updated commit sha1. New commits:
c185874  trac #16261: Merged with 6.3.beta1

comment:21 Changed 8 years ago by
Status:  needs_review → needs_work 

sage t long src/sage/combinat/designs/database.py # 4 doctests failed sage t long src/sage/combinat/designs/orthogonal_arrays.py # 2 doctests failed
comment:22 Changed 8 years ago by
Not on my computer.... Can you tell me which doctests fail ?
Nathann
comment:24 followup: 26 Changed 8 years ago by
Status:  needs_work → needs_review 

Sorry, the reason was more stupid than that : I had forgotten to recompile.
Btw, do you plan on reviewing all the tickets you set to needs_work
?
Nathann
comment:25 Changed 8 years ago by
Commit:  c1858741b33741bac325cf96a8166712273151bf → 890c448ad9ea547feb778b5f8e69f6bbac452f1d 

Branch pushed to git repo; I updated commit sha1. New commits:
890c448  trac #16261: Broken doctests

comment:26 Changed 8 years ago by
comment:27 Changed 8 years ago by
Milestone:  sage6.3 → sage6.4 

comment:28 Changed 8 years ago by
Cc:  Travis Scrimshaw added 

Yo Travis ! Now it is my turn :)
Nathann
comment:29 Changed 8 years ago by
Branch:  u/ncohen/16261 → public/groups/additive_abelian_groups_tuples16261 

Commit:  890c448ad9ea547feb778b5f8e69f6bbac452f1d → 36cf5008dd7712178b5b96ef38b9eb8518c7ddfb 
Reviewers:  → Travis Scrimshaw 
Hey Nathann,
I gave the short_repr
more functionaltype and cleaner (IMO) implementation. I've also made some lines shorter and made line breaks at more logical positions.
As for finding the orders of the original cyclic groups in short_repr
, I think the current implementation is okay for now. You could make this data into a (cached) method, but IDK if this is worthwhile or useful for anyone. A "better" fix would be to pass this data from the factory function to the class, but you should deal with handling old pickles which don't (a priori) contain this part of the class's input.
New commits:
aa17e43  Merge branch 'u/ncohen/16261' of trac.sagemath.org:sage into public/groups/additive_abelian_groups16261

36cf500  Better IMO short_repr method and made some lines shorter/logically line broken.

comment:30 Changed 8 years ago by
Status:  needs_review → positive_review 

Looks good ! Thank you for that. It is risky to touch combinat/designs/database.py these days but hopefully this won't conflict with anything as you only modify function at the middle of the code :P
Thanks for your help !
Nathann
comment:31 Changed 8 years ago by
Branch:  public/groups/additive_abelian_groups_tuples16261 → 36cf5008dd7712178b5b96ef38b9eb8518c7ddfb 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:32 Changed 8 years ago by
Commit:  36cf5008dd7712178b5b96ef38b9eb8518c7ddfb 

A question: with the old behavior, I could do
G = AdditiveAbelianGroup([0,0]) G(a)
and have it return something sensible (and the same thing) whether a
was a vector, a list, a tuple, or an element of G
. This now gives a deprecation warning if a
is a list or tuple. On the other hand
G.linear_combination_of_smith_form_gens(a)
gives an outright error if a
is an element of G
. Should I do
G(vector(a))
to recover this behavior?
comment:33 Changed 8 years ago by
I think you only got the same thing by coincidence, the Smith form generators happened to be chosen equal to the given basis. But we didn't make any guarantees that this was so, and for more complicated groups you'd be in for a surprise.
comment:34 Changed 8 years ago by
So if I'm working with free abelian groups, there is no way to guarantee that G(a)
will return the same thing if a
is the tuple (1,2), the list [1,2], the vector (1,2), or the group element (1,2)?
comment:35 Changed 8 years ago by
G(vector(a))
should work, I think. Do you want linear combinations of Smith form generators or linear combinations of given generators (if the group doesn't happen to be free)?
comment:36 Changed 8 years ago by
For my particular uses, I only want to work with free groups. If I happened to have a nonfree group, I think I would want linear combinations of given generators.
comment:37 followup: 38 Changed 7 years ago by
I bumped into this doctesting Judson's Abstract Algebra book.
G = AdditiveAbelianGroup([14]) b = G([2])
was fine for the novices, but b = G(vector([2]))
looks more complicated than necessary.
Anybody on this ticket have a better idea? I mean besides me ever getting my act together on #9773.
comment:38 Changed 7 years ago by
was fine for the novices, but
b = G(vector([2]))
looks more complicated than necessary.
What do you mean? G([2])
works in the latest release, and it will stay this way: we will only remove the *warning* in several months.
The meaning of the command changed, but the command itsels will remain available.
Nathann
comment:39 Changed 7 years ago by
OK, I understood the warning to mean behavior was changing and students were going to need to do some sort of linear combination drill. (Yes, I did skim through this ticket.) And of course the warning gets in the way for doctesting, but I know how to work around that cleanly.
Thanks very much for the quick and helpful reply, Nathann.
Rob
New commits:
trac #16261: Default behaviour of AdditiveAbelianGroup(a_tuple)