Doctest sage.categories.action

[roed314]

Currently there are no doctests in this file.

---

Apply attachment: 12039_doctests.patch, attachment: 12039_outside.patch, attachment: 12039_example.patch, attachment: 12039_changes_v2.patch

Depends on #12048

Component: doctest coverage

Keywords: sd40.5

Author: David Roe

Reviewer: Florent Hivert, David Loeffler, Karl-Dieter Crisman

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

[roed314]

Attachment: 12039_doctests.patch.gz

Doctests and changes sage.categories.action

[roed314]

Some changes outside sage.categories.action and sage.categories.action_example

[roed314]

Dependencies: #12048

[roed314]

comment:1

Attachment: 12039_outside.patch.gz

[roed314]

Author: David Roe

[hivert]

comment:2

I was googling for LazyFormat when I stumbled here.

One remark: There is no need to use LazyFormat if the result is a plain string. Just use the string itself. The purpose of LazyFormat is the following: when you have

lz=LazyFormat("... %s ...")%(obj)

it defers the computation of __repr__(obj) until one call __repr__(lz). So that,

LazyFormat("Message")%(())

is really some kind of overkill.

[hivert]

Reviewer: Florent Hivert

[hivert]

comment:3

A followup for my last remark. The idea of LazyFormat is to be able to
print counter examples in the error message for TestSuite when a property is
wrong, without drastically slowing down the tests. I would therefore suggest
to replace

tester.assertTrue(self(a, self(b, v)) == self(a*b, v), 
                  LazyFormat("Multiplication is not associative")%(())) 

by

tester.assertTrue(self(a, self(b, v)) == self(a*b, v), 
    LazyFormat("Multiplication is not associative: %s*(%s*%s) != (%s*%s)*%s")%(a,b,v,a,b,v))

And similarly for the other one.

Also in the category directory, the usage is to place the example in the
subdirectory examples. An action isn't really a category, but I think it would
be good to have all the examples there. Moreover, don't forget to add your
very nice example in the reference manual (file: doc/en/reference/categories.rst).

By the way: thanks a lot for writing all this nice documentation.

Florent

[hivert]

comment:4

More remarks:

• There is an empty Example:: section for _extend_G(self, K):.

• you should put hyperlinks ref for :class:`sage.matrix.action.MatrixMatrixAction` line 14 in sage/categories/action.pyx, as well as in

17	See sage.structure.coerce_actions and sage.matrix.action for examples of Actions  
18	used in the coercion system, and sage.structure.coerce and sage.structure.parent 
19	for the code that uses them. 

Note: I didn't look exhaustively for missing hyperlinks.

Sorry for asking all these modifications. More review tomorrow morning, I'm off to bed.

Florent

[roed314]

Attachment: 12039_example.patch.gz

An extended example of an Action

[roed314]

Incorporate referee's suggestions so far

[roed314]

comment:5

Attachment: 12039_changes.patch.gz

I don't mind making changes: I'm glad you're refereeing it.

[loefflerd]

comment:6

Doesn't apply to latest beta -- it gets as far as patch 12039_changes.patch and then that fails to apply.

[loefflerd]

Changed reviewer from Florent Hivert to Florent Hivert, PatchBot

[loefflerd]

comment:7

Aargh, patchbot is interpreting my last comment as an instruction to only apply the last patch...

Apply 12039_doctests.patch, 12039_outside.patch, 12039_example.patch, 12039_changes.patch

[loefflerd]

Attachment: 12039_changes_v2.patch.gz

Apply this instead of 12039-changes.patch

[loefflerd]

comment:8

The patch I've just uploaded is based on a rebase of 12039_changes.patch, but it also incorporates some reviewer changes: I've corrected the Sphinx formatting in a few places so it builds without errors, added some trivial missing doctests to get coverage up to 100%, and added a few hyperlinks and so on.

Apply 12039_doctests.patch, 12039_outside.patch, 12039_example.patch, 12039_changes_v2.patch

[roed314]

comment:9

Thanks for fixing all the action_example -> examples.actions. Your changes all look good to me. Feel free to give it a positive review if you're happy with it.

[loefflerd]

comment:10

I'll put this to positive review pending #12048. But is there any chance that you could modify this so it works without #12048? (I think without #12048 the patches apply correctly, but doctests don't pass.)

[loefflerd]

Changed reviewer from Florent Hivert, PatchBot to Florent Hivert, David Loeffler

[kcrisman]

comment:12

In Sage 5.1.beta0 I get a bunch of errors applying (yes, I did apply #12048)? Can you confirm this? Also, it would be really nice to have this not depend on a 'needs work' patch.

[kcrisman]

Changed reviewer from Florent Hivert, David Loeffler to Florent Hivert, David Loeffler, Karl-Dieter Crisman

[kcrisman]

Changed keywords from none to sd40.5

[kcrisman]

comment:13

Needs work, as discussed in person here - 5.0 is okay, 5.1.beta0 is not.

applying 12039_doctests.patch
patching file sage/categories/action.pxd
Hunk #1 FAILED at 0
1 out of 2 hunks FAILED -- saving rejects to file sage/categories/action.pxd.rej
patching file sage/categories/action.pyx
Hunk #1 FAILED at 6
Hunk #2 FAILED at 25
Hunk #3 FAILED at 41
Hunk #4 succeeded at 107 with fuzz 1 (offset 35 lines).
Hunk #5 FAILED at 119
Hunk #6 FAILED at 174
Hunk #10 FAILED at 391
6 out of 11 hunks FAILED -- saving rejects to file sage/categories/action.pyx.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 12039_doctests.patch

[jpflori]

comment:14

I guess the problem is that #715 was merged in 5.1.beta0
This ticket also brings notable changes to the Action class and adds a bunch of doc to the file, so rebasing this ticket might be non trivial (from what I've seen of the patches, the changes are even more intrusive than what was done in #715).