Opened 10 years ago

Closed 8 years ago

#9235 closed defect (fixed)

Doctest coverage for sage.categories.homset

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

Description (last modified by SimonKing)

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 10 years ago.
deprecation warnings for get_action_c and coerce_map_from_c
9235_doctest_homset.patch (7.9 KB) - added by SimonKing 8 years ago.
Almost full doctest coverage for homset.py
trac_9235-doctest_homset-review-ts.patch (10.5 KB) - added by tscrim 8 years ago.
Fixed formatting

Download all attachments as: .zip

Change History (20)

comment:1 Changed 10 years ago by SimonKing

  • Status changed from new to needs_review

comment:2 Changed 10 years ago by SimonKing

  • Status changed from needs_review to needs_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 10 years ago by SimonKing

  • Status changed from needs_work to needs_review

comment:4 Changed 10 years ago by niles

  • Cc niles added

comment:5 Changed 10 years ago by SimonKing

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 10 years ago by SimonKing

-- 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 10 years ago by niles

deprecation warnings for get_action_c and coerce_map_from_c

comment:7 Changed 10 years ago by niles

  • Description modified (diff)
  • Reviewers set to Niles Johnson
  • Status changed from needs_review to needs_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 10 years ago by niles

  • 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 10 years ago by niles

patchbot: apply 9235_doctest_homset.patch

comment:10 Changed 8 years ago by SimonKing

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 8 years ago by SimonKing

Almost full doctest coverage for homset.py

comment:11 Changed 8 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_work to needs_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 8 years ago by tscrim

  • Description modified (diff)
  • Keywords days45 added
  • Reviewers changed from Niles Johnson to Niles 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 8 years ago by SimonKing

  • 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 8 years ago by tscrim

Fixed formatting

comment:14 Changed 8 years ago by tscrim

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 8 years ago by SimonKing

  • Description modified (diff)
  • Status changed from needs_review to positive_review

The reviewer patch looks fine to me!

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

comment:16 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.7 to sage-5.8

comment:17 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.8.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.