Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#16261 closed enhancement (fixed)

Default behaviour of AdditiveAbelianGroup(a_tuple)

Reported by: Nathann Cohen Owned by:
Priority: major Milestone: sage-6.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:

Status badges

Description

This ticket follows the conversation on https://groups.google.com/forum/#!topic/sage-support/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)

doctest.txt (8.7 KB) - added by Ralf Stephan 9 years ago.
log of doctest fails

Download all attachments as: .zip

Change History (40)

comment:1 Changed 9 years ago by Nathann Cohen

Branch: u/ncohen/16261
Commit: 270545adf1dfc9e7554cb2f78700402f787a56b4

New commits:

270545atrac #16261: Default behaviour of AdditiveAbelianGroup(a_tuple)

comment:2 Changed 9 years ago by git

Commit: 270545adf1dfc9e7554cb2f78700402f787a56b4cdecd43022a6c78f40655374c3eb804b810483e9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cdecd43trac #16261: Default behaviour of AdditiveAbelianGroup(a_tuple)

comment:3 Changed 9 years ago by Nathann Cohen

Status: newneeds_review

comment:4 Changed 9 years ago by Volker Braun

Status: needs_reviewneeds_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 9 years ago by Nathann Cohen

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 9 years ago by Nathann Cohen

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 9 years ago by git

Commit: cdecd43022a6c78f40655374c3eb804b810483e904683ed9d981b1a561163867eee8f63044733fc9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

04683edtrac #16261: Default behaviour of AdditiveAbelianGroup(a_tuple)

comment:8 Changed 9 years ago by Nathann Cohen

Now passes all tests except one, see discussion on the sage-support thread.

Nathann

comment:9 Changed 9 years ago by git

Commit: 04683ed9d981b1a561163867eee8f63044733fc9fe241f151b7652552560d0867803309bc3e2f8e0

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

fe241f1trac #16261: Default behaviour of AdditiveAbelianGroup(a_tuple)

comment:10 Changed 9 years ago by Nathann Cohen

(all doctests pass !)

comment:11 Changed 9 years ago by git

Commit: fe241f151b7652552560d0867803309bc3e2f8e05d78cfc86f64819c0427941c882015dd2e3df73e

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

5d78cfctrac #16261: Default behaviour of AdditiveAbelianGroup(a_tuple)

comment:12 Changed 9 years ago by Nathann Cohen

Status: needs_workneeds_review

comment:13 Changed 9 years ago by Nils Bruin

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 ; Changed 9 years ago by Nathann Cohen

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 not Z^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 ; Changed 9 years ago by Nils Bruin

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 ZZ-modules anyway, why not do it properly?

comment:16 in reply to:  15 Changed 9 years ago by Nathann Cohen

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 ZZ-modules 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 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:18 Changed 9 years ago by Ralf Stephan

Work issues: merge conflict

comment:19 Changed 9 years ago by git

Commit: 5d78cfc86f64819c0427941c882015dd2e3df73ec1858741b33741bac325cf96a8166712273151bf

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

c185874trac #16261: Merged with 6.3.beta1

comment:20 Changed 9 years ago by Nathann Cohen

Work issues: merge conflict

Fixed.

Nathann

comment:21 Changed 9 years ago by Ralf Stephan

Status: needs_reviewneeds_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 9 years ago by Nathann Cohen

Not on my computer.... Can you tell me which doctests fail ?

Nathann

Changed 9 years ago by Ralf Stephan

Attachment: doctest.txt added

log of doctest fails

comment:23 Changed 9 years ago by Ralf Stephan

Log is attached. Hope you tested 6.3.beta1.

comment:24 Changed 9 years ago by Nathann Cohen

Status: needs_workneeds_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 9 years ago by git

Commit: c1858741b33741bac325cf96a8166712273151bf890c448ad9ea547feb778b5f8e69f6bbac452f1d

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

890c448trac #16261: Broken doctests

comment:26 in reply to:  24 Changed 9 years ago by Ralf Stephan

Replying to ncohen:

Btw, do you plan on reviewing all the tickets you set to needs_work ?

No.

comment:27 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:28 Changed 8 years ago by Nathann Cohen

Cc: Travis Scrimshaw added

Yo Travis ! Now it is my turn :-)

Nathann

comment:29 Changed 8 years ago by Travis Scrimshaw

Branch: u/ncohen/16261public/groups/additive_abelian_groups_tuples-16261
Commit: 890c448ad9ea547feb778b5f8e69f6bbac452f1d36cf5008dd7712178b5b96ef38b9eb8518c7ddfb
Reviewers: Travis Scrimshaw

Hey Nathann,

I gave the short_repr more functional-type 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:

aa17e43Merge branch 'u/ncohen/16261' of trac.sagemath.org:sage into public/groups/additive_abelian_groups-16261
36cf500Better IMO short_repr method and made some lines shorter/logically line broken.
Last edited 8 years ago by Travis Scrimshaw (previous) (diff)

comment:30 Changed 8 years ago by Nathann Cohen

Status: needs_reviewpositive_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 Volker Braun

Branch: public/groups/additive_abelian_groups_tuples-1626136cf5008dd7712178b5b96ef38b9eb8518c7ddfb
Resolution: fixed
Status: positive_reviewclosed

comment:32 Changed 8 years ago by John Palmieri

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 Volker Braun

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 John Palmieri

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 Volker Braun

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 John Palmieri

For my particular uses, I only want to work with free groups. If I happened to have a non-free group, I think I would want linear combinations of given generators.

comment:37 Changed 8 years ago by Rob Beezer

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 8 years ago by Nathann Cohen

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 8 years ago by Rob Beezer

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

Note: See TracTickets for help on using tickets.