Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#16261 closed enhancement (fixed)

Default behaviour of AdditiveAbelianGroup(a_tuple)

Reported by: ncohen Owned by:
Priority: major Milestone: sage-6.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/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 rws 6 years ago.
log of doctest fails

Download all attachments as: .zip

Change History (40)

comment:1 Changed 6 years ago by ncohen

  • Branch set to u/ncohen/16261
  • Commit set to 270545adf1dfc9e7554cb2f78700402f787a56b4

New commits:

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

comment:2 Changed 6 years ago by git

  • Commit changed from 270545adf1dfc9e7554cb2f78700402f787a56b4 to cdecd43022a6c78f40655374c3eb804b810483e9

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 6 years ago by ncohen

  • Status changed from new to needs_review

comment:4 Changed 6 years ago by vbraun

  • 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 6 years ago by ncohen

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 6 years ago by ncohen

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

  • Commit changed from cdecd43022a6c78f40655374c3eb804b810483e9 to 04683ed9d981b1a561163867eee8f63044733fc9

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 6 years ago by ncohen

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

Nathann

comment:9 Changed 6 years ago by git

  • Commit changed from 04683ed9d981b1a561163867eee8f63044733fc9 to fe241f151b7652552560d0867803309bc3e2f8e0

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 6 years ago by ncohen

(all doctests pass !)

comment:11 Changed 6 years ago by git

  • Commit changed from fe241f151b7652552560d0867803309bc3e2f8e0 to 5d78cfc86f64819c0427941c882015dd2e3df73e

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 6 years ago by ncohen

  • Status changed from needs_work to needs_review

comment:13 follow-up: Changed 6 years ago by nbruin

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 ; follow-up: Changed 6 years ago by ncohen

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 ; follow-up: Changed 6 years ago by nbruin

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 6 years ago by ncohen

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 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:18 Changed 6 years ago by rws

  • Work issues set to merge conflict

comment:19 Changed 6 years ago by git

  • Commit changed from 5d78cfc86f64819c0427941c882015dd2e3df73e to c1858741b33741bac325cf96a8166712273151bf

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

c185874trac #16261: Merged with 6.3.beta1

comment:20 Changed 6 years ago by ncohen

  • Work issues merge conflict deleted

Fixed.

Nathann

comment:21 Changed 6 years ago by rws

  • 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 6 years ago by ncohen

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

Nathann

Changed 6 years ago by rws

log of doctest fails

comment:23 Changed 6 years ago by rws

Log is attached. Hope you tested 6.3.beta1.

comment:24 follow-up: Changed 6 years ago by ncohen

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

  • Commit changed from c1858741b33741bac325cf96a8166712273151bf to 890c448ad9ea547feb778b5f8e69f6bbac452f1d

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

890c448trac #16261: Broken doctests

comment:26 in reply to: ↑ 24 Changed 6 years ago by rws

Replying to ncohen:

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

No.

comment:27 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:28 Changed 5 years ago by ncohen

  • Cc tscrim added

Yo Travis ! Now it is my turn :-)

Nathann

comment:29 Changed 5 years ago by tscrim

  • Branch changed from u/ncohen/16261 to public/groups/additive_abelian_groups_tuples-16261
  • Commit changed from 890c448ad9ea547feb778b5f8e69f6bbac452f1d to 36cf5008dd7712178b5b96ef38b9eb8518c7ddfb
  • Reviewers set to 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 5 years ago by tscrim (previous) (diff)

comment:30 Changed 5 years ago by ncohen

  • 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 5 years ago by vbraun

  • Branch changed from public/groups/additive_abelian_groups_tuples-16261 to 36cf5008dd7712178b5b96ef38b9eb8518c7ddfb
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:32 Changed 5 years ago by jhpalmieri

  • 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 5 years ago by vbraun

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 5 years ago by jhpalmieri

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 5 years ago by vbraun

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 5 years ago by jhpalmieri

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 follow-up: Changed 5 years ago by rbeezer

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 5 years ago by ncohen

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 5 years ago by rbeezer

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.