[with patch, positive review] make abelian group list/iter and classgroup list/iter more modern

Needs work. Your fix patch introduces a bug:

sage: G = AbelianGroup([2,3], names = "ab") sage: v = G.list()
sage: v
[1, b, b^2, a, a*b, a*b^2]
sage: v[0] = "hi nick!"
sage: G.list()
['hi nick!', b, b^2, a, a*b, a*b^2]

3810-ncalexan-abelian-group-iter-2.patch replaces 3810-ncalexan-abelian-group-iter.patch and addresses referee comment.

This is functionality which I wanted a few days ago so I am pleased to see this ticket!

I applied 3810-ncalexan-abelian-group-iter-2.patch and then 3810-ncalexan-class-group-list.patch in that order. When Sage applied the second one I was surprised that it popped up an edit window as when one does a commit. I just closed the window.

So I don't really know what it was I tested, but it seemed to work as advertised (and without the bug William pointed out).

Here's an enhancement I would like: If I create an ideal class from some random ideal, nothing really happens, and I cannot tell which class I got without testing for equality with every class. Am I missing something? I actually would have been happy to have class group elements display as abstract group elements (as they would in Magma), as long as functions were provided to go from fractional ideals to abstract ideal classes and back again.

With the two patches John mentioned I get the following doctest failure:

sage -t -long devel/sage/sage/rings/number_field/class_group.py
**********************************************************************
File "/scratch/mabshoff/release-cycle/sage-3.1.2.alpha1/tmp/class_group.py", line 117:
    sage: list(G)
Expected:
    [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^2 + a - 1/2), Fractional ideal class (2, 1/2*a^2 + 1/2)]
Got:
    [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^2 - a + 3/2), Fractional ideal class (2, 1/2*a^2 + 1/2)]
**********************************************************************
File "/scratch/mabshoff/release-cycle/sage-3.1.2.alpha1/tmp/class_group.py", line 119:
    sage: G.list()
Expected:
    [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^2 + a - 1/2), Fractional ideal class (2, 1/2*a^2 + 1/2)]
Got:
    [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^2 - a + 3/2), Fractional ideal class (2, 1/2*a^2 + 1/2)]
**********************************************************************

The reason that trac_3810-ncalexan-class-group-list.patch popped up a window is that it is a straight diff instead of a patch. Nick: Can you post a unified patch and delete the old ones?

Cheers,

Michael

Those doctest differences are insignificant in that the output is mathematically identical to what is expected. For deterministic output we need (as well as the usual sorting): the same ideals representing each class, and the same presentation of each ideal. Here it's just that the second ideal in the list has a different second generator.

My understanding is that these generators come from pari. If so, it is probably safe to replace the doctest output with the new output. Perhaps Nick could say if that sounds right to him.

Nick, any news on this? It might be relevant to #4061 which I am about to post a patch for.

I think these should be applied. The doctest output should be updated, but I don't have the original patch queue anymore to give a non-patch.

Just one thing is stopping me giving this the ok:

sage: G=AbelianGroup([])
sage: G.list()
[]
sage: list(G)
[]
sage: G.order()
1

Trivial groups do have one element! This special case is handled ok in the class groups patch, and a similar special case is needed for abelian groups. (It happens because mrange() delivers nothing at all for an empty list of lists, that is one valid way of defining a trivial abelian group.)

What's bugging me is that I discovered that same issue with listing trivial groups myself a few days ago, and somewhere else in trac there's a patch which does what is needed here -- but I cannot remember which one! So this might cause some merge problems when my patch goes in.

Apart from this, the second and third patches apply fine to 3.2.1 and tests pass. *Ignore the first patch which is replaced by the third!*

NB The patch at #4061 which has just been merged into 3.2.2 contains a fix for the trivial group list problem, so this ticket should be checked against that. I hope to get to that today. With luck it will mean that I can remove my one misgiving about this one!

Replaces all previous; apply to 3.2.1 + #4061 patches

I hope I have done this right. I took a clone of 3.2.1 and applied the (already merged) patches at #4061, which included a fix for trivial abelian group list(). I merged Nick's two patches from this ticket, adapting them accordingly and fixing the trivial group thing I mentioned above, adding a doctest. The result became the "combined" patch attached here. It looks strange when you view it because the file abelian_groups.py has two successive diffs, but I tried applying this patch to a fresh clone (+#4061) and it does end up in the right state.

If this makes sense to Michael, and works for him, I hope this will finish off this one.

I am seeing one doctest failure with this patch applied on a 64 bit Linux box:

sage -t -long "devel/sage/sage/rings/number_field/class_group.py"
**********************************************************************
File "/scratch/mabshoff/release-cycle/sage-3.2.2.alpha1/devel/sage/sage/rings/number_field/class_group.py", line 148, in __main__.example_8
Failed example:
    list(G)###line 117:_sage_    >>> list(G)
Expected:
    [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^2 + a - 1/2), Fractional ideal class (2, 1/2*a^2 + 1/2)]
Got:
    [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^2 - a + 3/2), Fractional ideal class (2, 1/2*a^2 + 1/2)]
**********************************************************************
File "/scratch/mabshoff/release-cycle/sage-3.2.2.alpha1/devel/sage/sage/rings/number_field/class_group.py", line 150, in __main__.example_8
Failed example:
    G.list()###line 119:_sage_    >>> G.list()
Expected:
    [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^2 + a - 1/2), Fractional ideal class (2, 1/2*a^2 + 1/2)]
Got:
    [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^2 - a + 3/2), Fractional ideal class (2, 1/2*a^2 + 1/2)]
**********************************************************************

Replying to mabshoff:

I am seeing one doctest failure with this patch applied on a 64 bit Linux box: {{{ sage -t -long "devel/sage/sage/rings/number_field/class_group.py" ********************************************************************** File "/scratch/mabshoff/release-cycle/sage-3.2.2.alpha1/devel/sage/sage/rings/number_field/class_group.py", line 148, in main.example_8 Failed example: list(G)###line 117:_sage_ >>> list(G) Expected: [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^{2 + a - 1/2), Fractional ideal class (2, 1/2*a}2 + 1/2)] Got: [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^{2 - a + 3/2), Fractional ideal class (2, 1/2*a}2 + 1/2)] ********************************************************************** File "/scratch/mabshoff/release-cycle/sage-3.2.2.alpha1/devel/sage/sage/rings/number_field/class_group.py", line 150, in main.example_8 Failed example: G.list()###line 119:_sage_ >>> G.list() Expected: [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^{2 + a - 1/2), Fractional ideal class (2, 1/2*a}2 + 1/2)] Got: [Trivial principal fractional ideal class, Fractional ideal class (2, 1/2*a^{2 - a + 3/2), Fractional ideal class (2, 1/2*a}2 + 1/2)] ********************************************************************** }}}

What a nuisance; sorry, I only did a 32-bit test at the weekend. The got/expected are equally valid mathematically, and my guess is that the difference comes from the pari library.

Would it be dishonest to look for an example for the doctest where the output was identical on 32- and 64-bit machines?

Replying to cremona:

Hi John,

What a nuisance; sorry, I only did a 32-bit test at the weekend. The got/expected are equally valid mathematically, and my guess is that the difference comes from the pari library.

Yes, Nick said the same thing in IRC, but I was asleep at that point.

Would it be dishonest to look for an example for the doctest where the output was identical on 32- and 64-bit machines?

I am fine marking those two doctests with #32 bit and #64 bit output and to get the patch in. If anybody wants to have a "prettier" doctest that can be addressed via a followup patch.

Cheers,

Michael

Replying to mabshoff:

Replying to cremona: Hi John,

What a nuisance; sorry, I only did a 32-bit test at the weekend. The got/expected are equally valid mathematically, and my guess is that the difference comes from the pari library.

Yes, Nick said the same thing in IRC, but I was asleep at that point.

Would it be dishonest to look for an example for the doctest where the output was identical on 32- and 64-bit machines?

I am fine marking those two doctests with #32 bit and #64 bit output and to get the patch in. If anybody wants to have a "prettier" doctest that can be addressed via a followup patch.

Sounds good to me. If it is more random than that it will be picked up by the merry horde of alpha testers....

John

Cheers, Michael

New patch fixes the 32/64 problem, has been tested on both.

Merged sage-3810-combined.patch and sage-3810-fix64.patch in Sage 3.2.2.alpha1