Opened 12 years ago

Closed 10 years ago

#9235 closed defect (fixed)

Doctest coverage for sage.categories.homset

Reported by: Simon King Owned by: Nicolas M. Thiéry
Priority: minor Milestone: sage-5.8
Component: categories Keywords: doctest coverage homset, days45
Cc: Niles Johnson Merged in: sage-5.8.beta0
Authors: Simon King Reviewers: Niles Johnson, Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Simon King)

The doctest coverage for sage.categories.homset was

SCORE devel/sage-main/sage/categories/homset.py: 52% (13 of 25)

My patch covers all but two methods:

  • get_action_c
  • coerce_map_from_c

These two return (by default) None. Is there a good indirect doctest for these two? I am not familiar with get_action, and I don't know what coerce_map_from_c does, compared with _coerce_map_from_. Perhaps the reviewer can explain it to me, so that I or s/he can add the two missing tests?

Apply

  1. 9235_doctest_homset.patch
  2. trac_9235-doctest_homset-review-ts.patch

Attachments (3)

9235-reviewer.patch (1.5 KB) - added by Niles Johnson 12 years ago.
deprecation warnings for get_action_c and coerce_map_from_c
9235_doctest_homset.patch (7.9 KB) - added by Simon King 10 years ago.
Almost full doctest coverage for homset.py
trac_9235-doctest_homset-review-ts.patch (10.5 KB) - added by Travis Scrimshaw 10 years ago.
Fixed formatting

Download all attachments as: .zip

Change History (20)

comment:1 Changed 12 years ago by Simon King

Status: newneeds_review

comment:2 Changed 12 years ago by Simon King

Status: needs_reviewneeds_work

I can not reproduce the error that the patchbot recently found. I don't know how I can push it to test the patch again.

So, I'll change into needs-work and then immediately into needs-review - hope that triggers another attempt of the bot...

comment:3 Changed 12 years ago by Simon King

Status: needs_workneeds_review

comment:4 Changed 12 years ago by Niles Johnson

Cc: Niles Johnson added

comment:5 Changed 12 years ago by Simon King

It seems to me that the doctest error found by the patchbot is unrelated with my patch: After all, the patch is a just adding documentation and tests.

Is anybody inclined to review it?

comment:6 Changed 12 years ago by Simon King

-- bump --

The patch is in need of a review since one year. I think it would not be difficult to review a pure doctest patch. But having full doctest coverage in yet another part of Sage would be good to have.

I hope that the patch still applies.

Changed 12 years ago by Niles Johnson

Attachment: 9235-reviewer.patch added

deprecation warnings for get_action_c and coerce_map_from_c

comment:7 Changed 12 years ago by Niles Johnson

Description: modified (diff)
Reviewers: Niles Johnson
Status: needs_reviewneeds_work

You're right -- this shouldn't have to wait so long! I've looked through all the changes, and they look good! All tests pass, and the documentation builds cleanly, without warnings.

I used search_src to look for other places in Sage where get_action_c and coerce_map_from_c are used. The only places they appear are in structure/parent_old, so I think these in Homset should be deprecated and (later) removed. I've added a reviewer patch which adds deprecation warnings and corresponding tests, raising the coverage to 100%.

The only issue I have with 9235_doctest_homset.patch is the following block:

    if category is None:
        if cat_X.is_subcategory(cat_Y):
            category = cat_Y
        elif cat_Y.is_subcategory(cat_X):
            # NT: this "category is None" test is useless and could be removed
            # SK: Indeed! For that reason, the ValueError would never be raised
            # NT: why is there an assymmetry between X and Y?
            # SK: I see no reason. In particular, I don't see why an error should
            #     be raised if cat_X is not cat_Y. So, I uncomment the following
            #     two lines.
##             if not (category is None) and not (cat_X is cat_Y):
##                 raise ValueError, "No unambiguous category found for Hom from %s to %s."%(X,Y)
            category = cat_X
        else:
            # Search for the lowest common super category
            subcats_X = cat_X.all_super_categories(proper = True)
            subcats_Y = set(cat_Y.all_super_categories(proper = True))
            category = None
            for c in subcats_X:
                if c in subcats_Y:
                    category = c
                    break

            if category is None:
                raise TypeError, "No suitable category found for Hom from %s to %s."%(X,Y)

If there's no reason to include the second "category is None" test, then it and the previous comments should simply be deleted. And there is a third "category is None" test in this block which also looks redundant.

comment:8 Changed 12 years ago by Niles Johnson

Description: modified (diff)

I see that issuing the deprecation warning causes some failures in modular/abvar/homspace.py, because the deprecation message is printed. A minimal example to produce the message is:

sage: J = J0(37)
sage: E = J.endomorphism_ring()
sage: x = -1*E.gens()[0]

But I don't understand any more about this, so maybe it's better not to include the deprecation warning. One could simply include tests of the form

sage: H = Hom(ZZ^2, ZZ^3)
sage: H.get_action_c(ZZ,operator.add,ZZ) is None
True

sage: H = Hom(ZZ^2, ZZ^3)
sage: H.coerce_map_from_c(ZZ) is None
True

without a deprecation warning.

Note that *removing* the methods get_action_c and coerce_map_from_c causes all tests in modular/abvar/homspace.py to pass (of course it should, since these don't do anything anyway). No other bit of Sage code even caused the deprecation warning to be raised, so perhaps removing them really is a good idea (in which case a deprecation warning would be the first step). But maybe this should be left for another ticket -- I'll leave it up to you at this point, Simon.

comment:9 Changed 12 years ago by Niles Johnson

patchbot: apply 9235_doctest_homset.patch

comment:10 Changed 10 years ago by Simon King

Again, a long time has passed, and meanwhile the patch doesn't apply (3 out of 8 hunks fail). I'll see what I can do about it.

Changed 10 years ago by Simon King

Attachment: 9235_doctest_homset.patch added

Almost full doctest coverage for homset.py

comment:11 Changed 10 years ago by Simon King

Description: modified (diff)
Status: needs_workneeds_review

The patch is updated. I think that introducing a deprecation warning for the two survivors of the old coercion model should be the subject of a new ticket. This here should be "doctests only".

Therefore:

Apply 9235_doctest_homset.patch

comment:12 Changed 10 years ago by Travis Scrimshaw

Description: modified (diff)
Keywords: days45 added
Reviewers: Niles JohnsonNiles Johnson, Travis Scrimshaw

Hey Simon,

I've uploaded a review patch which goes through and brings the rest of the documentation up to current standards and added the tests to the old coercion model methods with nice warning blocks. Otherwise looks good and I think we should push the actual deprecations to when we completely remove the old coercion model. If you agree with my changes, feel free to set this to positive review.

Thanks,
Travis

comment:13 Changed 10 years ago by Simon King

Description: modified (diff)

The patchbot has correctly applied both patches (I just checked) and finds that all tests pass. Since the patch changes formatting ("EXAMPLE:" becomes "EXAMPLES::" with double colon), I built the documentation.

I apologize for some misformatting that my patch has introduced (e.g., in codomain()). This is not fixed yet.

What shall we do? Shall I fix the misformattings in my patch? Or shall you fix it by updating the reviewer patch?

Changed 10 years ago by Travis Scrimshaw

Fixed formatting

comment:14 Changed 10 years ago by Travis Scrimshaw

Can't believe I missed that...this is why one should never read over a doc for errors past midnight :P

I've updated my review patch; thanks for catching that.

comment:15 Changed 10 years ago by Simon King

Description: modified (diff)
Status: needs_reviewpositive_review

The reviewer patch looks fine to me!

Apply 9235_doctest_homset.patch trac_9235-doctest_homset-review-ts.patch

comment:16 Changed 10 years ago by Jeroen Demeyer

Milestone: sage-5.7sage-5.8

comment:17 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.8.beta0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.