Bring doctests of modular/modsym/ambient.py up to 100%

[JohnCremona]

This 2500-line file has a low score:
26% (26 of 97)

I have nearly finished documenting it and will upload a patch over the weekend.

Component: modular forms

Keywords: modular symbols

Author: John Cremona

Reviewer: David Loeffler, Alex Ghitza

Merged: 4.0.rc0

Issue created by migration from https://trac.sagemath.org/ticket/6042

[JohnCremona]

Applies to 3.4.2

[JohnCremona]

comment:1

Attachment: trac_6042.patch.gz

This is just sickening. I must have spent over 10 hours in the last week documenting this file, resulting in a patch over 2000 lines long, and 100% doctest coverage. But now I cannot apply it since after the 7 patches on 3 tickets which David warned me about (#5736, #4357 and #5250 all already merged in 4.0alpha0, in that order) I get this mess:

john@ubuntu%sage -hg qpush
applying trac_6042.patch
patching file sage/modular/modsym/ambient.py
Hunk #17 FAILED at 800
Hunk #25 FAILED at 1291
Hunk #45 FAILED at 2230
Hunk #60 FAILED at 2829
Hunk #61 FAILED at 2836
Hunk #65 FAILED at 2962
Hunk #71 FAILED at 3195
7 out of 76 hunks FAILED -- saving rejects to file sage/modular/modsym/ambient.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
Errors during apply, please fix and refresh trac_6042.patch

I'm really not sure I can be bothered to mess with this any more. Is there any system to actually help one merge conflicting patches sensibly? I have never managed to get things like k3diff to work.

I will at least upload my patch so that it does not get lost, but I have other things to do.

[aghitza]

comment:2

Replying to @JohnCremona:

This is just sickening. I must have spent over 10 hours in the last week documenting this file, resulting in a patch over 2000 lines long, and 100% doctest coverage. But now I cannot apply it since after the 7 patches on 3 tickets which David warned me about (#5736, #4357 and #5250 all already merged in 4.0alpha0, in that order) I get this mess:

I will at least upload my patch so that it does not get lost, but I have other things to do.

Hi John,

I feel your pain. I don't know of an automated system to get this done properly. But I'll try to rebase it to 4.0alpha0, and review it in the process. Hopefully I can do this in the next 36 hours or so.

[loefflerd]

comment:3

Alex: have you already started on this? (I'm not sure what time zone you're in.) I ask because I'm probably the best placed person to sort it out, as the changes which have caused the conflicts were mine, which also puts me under a certain moral obligation as well :-). I don't want to tread on your toes if you've already done it so let me know if you have, but if not, leave it to me and I'll sort it out tomorrow morning.

[aghitza]

comment:4

I have started but haven't gotten very far yet, so I won't be upset if you want to do it. Most of the stuff is fairly straightforward, but there are some functions where you introduced documentation and examples, and then John's patch is doing things all over again. So that would best be done by one of you.

[loefflerd]

comment:5

I've rebased the patch to 4.0.alpha0 and checked that it passes doctests, and had a look through the code, and it all looks fine -- let's get this in without further delay!

[JohnCremona]

comment:6

Thanks David (and Alex)!

[JohnCremona]

comment:7

BTW, I see that "rebasing" has included "reattributing credit in the patch header"! :)

[sagetrac-mabshoff]

comment:8

Replying to @JohnCremona:

BTW, I see that "rebasing" has included "reattributing credit in the patch header"! :)

Hehe, I will fix this once I import the patch.

David: Before posting the patch you can just edit the credit in the hg header at the top of the file.

Cheers,

Michael

[JohnCremona]

comment:9

Replying to @sagetrac-mabshoff:

Replying to @JohnCremona:

BTW, I see that "rebasing" has included "reattributing credit in the patch header"! :)

Hehe, I will fix this once I import the patch.

David: Before posting the patch you can just edit the credit in the hg header at the top of the file.

Cheers,

Michael

Of course I would like credit to go to David too, if hg will allow.

[loefflerd]

Attachment: trac_6042-rebase.patch.gz

patch against 4.0.alpha0

[loefflerd]

comment:10

I just found out about "qrefresh -u", so I can masquerade as anybody I like. I'll now attribute any patches that don't work to some unsuspecting victim :-) I've uploaded a new version with credit correctly attributed to John.

[JohnCremona]

comment:11

Replying to @loefflerd:

I just found out about "qrefresh -u", so I can masquerade as anybody I like. I'll now attribute any patches that don't work to some unsuspecting victim :-) I've uploaded a new version with credit correctly attributed to John.

thanks -- I don't really mind, of course, but I was looking when I made sure that yours was a replacement patch and not a second patch.

[sagetrac-mabshoff]

comment:12

The rebased patch on top of two other trivial tickets in my 4.0.rc0 merge tree:

Overall weighted coverage score:  75.0%
Total number of functions:  22100
We need    6 more function to get to 75% coverage.

:))

Cheers,

Michael

[JohnCremona]

comment:13

That makes it all worth while!

[sagetrac-mabshoff]

comment:14

Merged in Sage 4.0.rc0.

Cheers,

Michael

[loefflerd]

Merged: 4.0.rc0

[loefflerd]

Reviewer: David Loeffler, Alex Ghitza

[loefflerd]

Author: John Cremona