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

Priority:  major  Milestone:  sage6.4 
Component:  group theory  Keywords:  
Cc:  vbraun, jhpalmieri, SimonKing, tscrim  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  36cf500 (Commits)  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 5 years ago by
 Branch set to u/ncohen/16261
 Commit set to 270545adf1dfc9e7554cb2f78700402f787a56b4
comment:2 Changed 5 years ago by
 Commit changed from 270545adf1dfc9e7554cb2f78700402f787a56b4 to 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 5 years ago by
 Status changed from new to needs_review
comment:4 Changed 5 years ago by
 Status changed from needs_review to 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 5 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 5 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 5 years ago by
 Commit changed from cdecd43022a6c78f40655374c3eb804b810483e9 to 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 5 years ago by
Now passes all tests except one, see discussion on the sagesupport thread.
Nathann
comment:9 Changed 5 years ago by
 Commit changed from 04683ed9d981b1a561163867eee8f63044733fc9 to 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:10 Changed 5 years ago by
(all doctests pass !)
comment:11 Changed 5 years ago by
 Commit changed from fe241f151b7652552560d0867803309bc3e2f8e0 to 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 5 years ago by
 Status changed from needs_work to needs_review
comment:13 followup: ↓ 14 Changed 5 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 in reply to: ↑ 13 ; followup: ↓ 15 Changed 5 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 in reply to: ↑ 14 ; followup: ↓ 16 Changed 5 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 in reply to: ↑ 15 Changed 5 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 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:18 Changed 5 years ago by
 Work issues set to merge conflict
comment:19 Changed 5 years ago by
 Commit changed from 5d78cfc86f64819c0427941c882015dd2e3df73e to c1858741b33741bac325cf96a8166712273151bf
Branch pushed to git repo; I updated commit sha1. New commits:
c185874  trac #16261: Merged with 6.3.beta1

comment:21 Changed 5 years ago by
 Status changed from needs_review to 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 5 years ago by
Not on my computer.... Can you tell me which doctests fail ?
Nathann
comment:23 Changed 5 years ago by
Log is attached. Hope you tested 6.3.beta1.
comment:24 followup: ↓ 26 Changed 5 years ago by
 Status changed from needs_work to 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 5 years ago by
 Commit changed from c1858741b33741bac325cf96a8166712273151bf to 890c448ad9ea547feb778b5f8e69f6bbac452f1d
Branch pushed to git repo; I updated commit sha1. New commits:
890c448  trac #16261: Broken doctests

comment:26 in reply to: ↑ 24 Changed 5 years ago by
comment:27 Changed 4 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:29 Changed 4 years ago by
 Branch changed from u/ncohen/16261 to public/groups/additive_abelian_groups_tuples16261
 Commit changed from 890c448ad9ea547feb778b5f8e69f6bbac452f1d to 36cf5008dd7712178b5b96ef38b9eb8518c7ddfb
 Reviewers set to 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.
So in the end, if you're okay with my changes, then positive review.
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 4 years ago by
 Status changed from needs_review to 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 4 years ago by
 Branch changed from public/groups/additive_abelian_groups_tuples16261 to 36cf5008dd7712178b5b96ef38b9eb8518c7ddfb
 Resolution set to fixed
 Status changed from positive_review to closed
comment:32 Changed 4 years ago by
 Commit 36cf5008dd7712178b5b96ef38b9eb8518c7ddfb deleted
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 4 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 4 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 4 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 4 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 4 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 in reply to: ↑ 37 Changed 4 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 4 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)