Farey symbols

[daveloeffler]

I just played around a bit with the new code for Farey symbols in
4.7.2.alpha3 (from ticket #11709).

I hate to do this, but I'm going to have to make a public stand here:
I think that this code should not have been given a positive review.

Among other things, the patch's documentation makes absolute howling
errors, like asserting that the intersection of Gamma0(8) and
Gamma1(4) is a non-congruence subgroup; it introduces the new class
sage.modular.arithgroup.noncongruence_example.GPrime, which has *no
usable methods whatsoever* (and fails the standard loads/dumps test);
and one of the new files added by the patch gets a doctest score of
0%.

This is fundamentally sound code, providing efficient implementations
of algorithms which will be extremely useful to have in Sage. My beef
here is with the reviewer, not the author. I think the Sage reviewing
process has failed here, and the code has been allowed in when it is
not polished enough to be included.

We cannot ship this code as part of a Sage release. I propose to open
a new ticket to fix all of the manifold brokenness in the Farey
symbols code (and to delete, with extreme prejudice, the
noncongruence_example.py file), and to make it a blocker for 4.7.2.
And I sincerely hope that the reviewer -- you know who you are -- will
apply a modicum of mathematical and programming sense when reviewing
patches in future.

David Loeffler

[William Stein]

On Thu, Sep 29, 2011 at 2:46 PM, daveloeffler <dave.l...@gmail.com> wrote:
> I just played around a bit with the new code for Farey symbols in
> 4.7.2.alpha3 (from ticket #11709).
>
> I hate to do this, but I'm going to have to make a public stand here:
> I think that this code should not have been given a positive review.
>
> Among other things, the patch's documentation makes absolute howling
> errors, like asserting that the intersection of Gamma0(8) and
> Gamma1(4) is a non-congruence subgroup; it introduces the new class
> sage.modular.arithgroup.noncongruence_example.GPrime, which has *no
> usable methods whatsoever* (and fails the standard loads/dumps test);
> and one of the new files added by the patch gets a doctest score of
> 0%.

Wow, that's really bad. The merger script (?) should at least check
for 100% coverage. That file has 0%. Bad. Regarding the
mathematics, that's really bad.

>
> This is fundamentally sound code, providing efficient implementations
> of algorithms which will be extremely useful to have in Sage. My beef
> here is with the reviewer, not the author. I think the Sage reviewing
> process has failed here, and the code has been allowed in when it is
> not polished enough to be included.

I'm glad you spotted it!!!

>
> We cannot ship this code as part of a Sage release. I propose to open
> a new ticket to fix all of the manifold brokenness in the Farey
> symbols code (and to delete, with extreme prejudice, the
> noncongruence_example.py file), and to make it a blocker for 4.7.2.

I agree.

> And I sincerely hope that the reviewer -- you know who you are -- will
> apply a modicum of mathematical and programming sense when reviewing
> patches in future.

I agree, though I would be less mean, since it's all public, and I
really hope that he continues to contribute to Sage, etc.

>
> David Loeffler
>
> --
> To post to this group, send an email to sage-...@googlegroups.com
> To unsubscribe from this group, send an email to sage-devel+...@googlegroups.com
> For more options, visit this group at http://groups.google.com/group/sage-devel
> URL: http://www.sagemath.org
>

--
William Stein
Professor of Mathematics
University of Washington
http://wstein.org

[Martin Raum]

I am sorry that you feel this ticket wasn't reviewed correctly. I
would hope William's policy of not being mean is adopted also by you.
While I, indeed, missed the noncongruence mistake (and yes, that was a
stupid one, but on the other hand it is a single documentation typo in
thousands of lines of code), the situation is different with GPrime.
Following the ticket's discussion (there was also a lot of
communication in private), you might notice that at first there was
nothing like GPrime. It was rather added quite late, after I pointed
out that at least a virtual class (in the sense of C++) showing how to
implement a more general group would be useful. In that sense the
class does nothing and for this reason, the class' docsting seemed
sufficient to me.

I notice that at least two (influencial) Sage developers disagree. And
I feel honestly sorry. One could certainly add docstrings, and it
seems that this is what William insists on. But the point seems, that
David doesn't like virtual classes for demonstration purpose. Let's
make this constructive: What would, according to you, serve the
purpose of illustrating how to implement more general groups?

On 30 Sep., 06:32, William Stein <wst...@gmail.com> wrote:

> > For more options, visit this group athttp://groups.google.com/group/sage-devel

[William Stein]

On Fri, Sep 30, 2011 at 9:47 AM, Martin Raum
<Marti...@matha.rwth-aachen.de> wrote:
> I am sorry that you feel this ticket wasn't reviewed correctly. I
> would hope William's policy of not being mean is adopted also by you.
> While I, indeed, missed the noncongruence mistake (and yes, that was a
> stupid one, but on the other hand it is a single documentation typo in
> thousands of lines of code), the situation is different with GPrime.
> Following the ticket's discussion (there was also a lot of
> communication in private), you might notice that at first there was
> nothing like GPrime. It was rather added quite late, after I pointed
> out that at least a virtual class (in the sense of C++) showing how to
> implement a more general group would be useful. In that sense the
> class does nothing and for this reason, the class' docsting seemed
> sufficient to me.
>
> I notice that at least two (influencial) Sage developers disagree. And
> I feel honestly sorry. One could certainly add docstrings, and it
> seems that this is what William insists on. But the point seems, that
> David doesn't like virtual classes for demonstration purpose. Let's
> make this constructive: What would, according to you, serve the
> purpose of illustrating how to implement more general groups?

Quick note: It is a policy since 2007 that every single new function
added to the Sage library must have a docstring with doctests
illustrating its usage. This includes "private" _ and __ methods.
It's just something we decided on long ago. Hopefully this
requirement is spelled out in the developer guide.

-- William

> For more options, visit this group at http://groups.google.com/group/sage-devel

[daveloeffler]

On Sep 30, 5:47 pm, Martin Raum <Martin.R...@matha.rwth-aachen.de>
wrote:

> I am sorry that you feel this ticket wasn't reviewed correctly. I
> would hope William's policy of not being mean is adopted also by you.

I apologise for my harshness yesterday. As you can probably
understand, I have put a great deal of work into Sage's modular forms
code over the years -- especially the code for arithmetic subgroups of
SL2Z -- and hence I have come to feel rather strongly about it.

> While I, indeed, missed the noncongruence mistake (and yes, that was a
> stupid one, but on the other hand it is a single documentation typo in
> thousands of lines of code),

I don't quite agree: the entire layout, even down to the name of the
file ("noncongruence_example"), leads you to expect that a non-
congruence subgroup is being defined -- and then out pops a subgroup
which is clearly congruence. We're not talking about a typo here:
we're talking about a glaring mathematical error.

> the situation is different with GPrime.
> Following the ticket's discussion (there was also a lot of
> communication in private), you might notice that at first there was
> nothing like GPrime. It was rather added quite late, after I pointed
> out that at least a virtual class (in the sense of C++) showing how to
> implement a more general group would be useful. In that sense the
> class does nothing and for this reason, the class' docsting seemed
> sufficient to me.

The point here is that Sage already has the facility to work with
completely arbitrary finite-index subgroups of PSL2Z (and now, since
#11422, of SL2Z). It would have been *less* work, and vastly more
useful, to check that the Farey symbols code worked properly with a
few examples of subgroups that aren't of the special types it handles
separately, using the existing implementations of those subgroups in
Sage, rather than adding this stub class -- perhaps useful to
developers but utterly useless for the end user. (The fact that this
group GPrime is actually just GammaH(8, [5]), and thus is actually
covered by one of Hartmut's more specific optimised implementations,
makes the way this stub has been created seem particularly bizarre.)

> I notice that at least two (influencial) Sage developers disagree. And
> I feel honestly sorry. One could certainly add docstrings, and it
> seems that this is what William insists on.

As William points out, it is a sage policy, not some private obsession
of mine (or of his). I think all of us have heartily cursed this
policy, after hours spent laboriously adding doctests to dozens of
functions whose entire content is "raise NotImplementedError"; but
without it the project would probably have collapsed long ago.

> But the point seems, that
> David doesn't like virtual classes for demonstration purpose. Let's
> make this constructive: What would, according to you, serve the
> purpose of illustrating how to implement more general groups?

Why illustrate how to do something, when it is less work to just do
it?

I have nothing against demonstration classes as such. in fact if you
look at the version history of the arithmetic subgroups code, you'll
see that the code for the principal congruence subgroups Gamma(n) was
originally only about ten lines long. Like your GPrime, the class
directly implemented hardly anything except _repr_ and a containment
check. Unlike your GPrime, it was implemented as a subclass of the
generic ArithmeticSubgroup class, so it automatically picked up a
considerable variety of ready-made methods and an interface compatible
with the other Sage arithmetic subgroup classes, making it infinitely
more useful to end users. The proof of that is that people have since
started using it in production code and complaining that it's rather
slow, leading Ron Evans to flesh out the class with some vastly
quicker Gamma(n)-specific methods for cusp enumeration and the like.

David

[William Stein]

On Fri, Sep 30, 2011 at 11:31 AM, daveloeffler <dave.l...@gmail.com> wrote:
>
> On Sep 30, 5:47 pm, Martin Raum <Martin.R...@matha.rwth-aachen.de>
> wrote:
>> I am sorry that you feel this ticket wasn't reviewed correctly. I
>> would hope William's policy of not being mean is adopted also by you.
>
> I apologise for my harshness yesterday. As you can probably
> understand, I have put a great deal of work into Sage's modular forms
> code over the years -- especially the code for arithmetic subgroups of
> SL2Z -- and hence I have come to feel rather strongly about it.
>
>> While I, indeed, missed the noncongruence mistake (and yes, that was a
>> stupid one, but on the other hand it is a single documentation typo in
>> thousands of lines of code),
>
> I don't quite agree: the entire layout, even down to the name of the
> file ("noncongruence_example"), leads you to expect that a non-
> congruence subgroup is being defined -- and then out pops a subgroup
> which is clearly congruence. We're not talking about a typo here:
> we're talking about a glaring mathematical error.

Just to confirm this -- The actual code for containment checking for
this "noncongruence" subgroup is a 1-liner that checks some congruence
conditions.

Yes, it is a major pain in the ass, and sometimes developers (like me)
have even refused to submit code to Sage because of it. Psage is
one outlet for such folks: http://code.google.com/p/purplesage/

But for stable production code (like Sage is meant to be!), having
100% doctest coverage is incredibly useful.

-- William