Badly formed error messages for ModularFormsRing constructor

[itaibn]

When you give the ModularFormsRing constructor invalid input, it returns a badly formed error message, as follows:

sage: ModularFormsRing(3.4)                                      
...
ValueError: Group (=%s) should be a congruence subgroup
sage: ModularFormsRing(Gamma0(2), base_ring=PolynomialRing(ZZ,x))
...
ValueError: Base ring (=%s) should be QQ, ZZ or a finite prime field

Component: modular forms

Author: John Cremona

Reviewer: Frédéric Chapoton

Merged: sage-5.12.beta4

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

[JohnCremona]

applies to 5.12.beta1

[JohnCremona]

comment:1

Attachment: trac_15037-modular.patch.gz

The patch fixes the problem and adds two doctests.

[JohnCremona]

Author: John Cremona

[fchapoton]

Reviewer: Frédéric Chapoton

[fchapoton]

comment:2

Attachment: trac_15037_review.patch.gz

here is a review patch. You can set the ticket to positive review if you agree with my changes.

[JohnCremona]

comment:3

Replying to @fchapoton:

here is a review patch. You can set the ticket to positive review if you agree with my changes.

OK, I am happy (though no more happy than I was before! Is it stated somewhere that the % sign should have spaces around it? If so I will obey in future!)

[fchapoton]

comment:4

well, this is the pep8 standard:

http://www.python.org/dev/peps/pep-0008/

and it is recommended for sage:

http://www.sagemath.org/doc/developer/conventions.html

[jdemeyer]

comment:6

Sorry to nitpick, but those exceptions should probably be TypeError, not ValueError (feel free to ignore this comment, it's not that important anyway).

[JohnCremona]

comment:7

I could easily change that, and at the same time include chapoton's 4 spaces into my patch, but are you sure? If I give a ring as base_ring which happens not to be one of the rings allowed, that is surely an error in its Value not its Type?

[jdemeyer]

comment:8

Replying to @JohnCremona:

I could easily change that, and at the same time include chapoton's 4 spaces into my patch, but are you sure? If I give a ring as base_ring which happens not to be one of the rings allowed, that is surely an error in its Value not its Type?

Well a ring like RR is really a type which is not allowed, so that would be clearly a TypeError. Perhaps you could argue that GF(9) should be a ValueError since it's a finite field (an allowed type) but with the wrong number of elements. But for simplicity, I think a TypeError there would be okay. Technically speaking, even GF(3) and GF(9) are different Python types, so the TypeError in all cases is certainly defensible:

sage: type(GF(3))
<class 'sage.rings.finite_rings.finite_field_prime_modn.FiniteField_prime_modn_with_category'>
sage: type(GF(9,'a'))
<class 'sage.rings.finite_rings.finite_field_givaro.FiniteField_givaro_with_category'>

[JohnCremona]

comment:9

OK -- but I don't think anyone seeing the error message would be very concerned over this distinction!

[jdemeyer]

comment:10

Replying to @JohnCremona:

OK -- but I don't think anyone seeing the error message would be very concerned over this distinction!

Of course, I clearly said in [comment:6] that it wasn't an important thing, just something I felt I should mention.

[jdemeyer]

Merged: sage-5.12.beta4