#11422: modular subgroups
--------------------------------+-------------------------------------------
Reporter: vdelecroix | Owner: vdelecroix
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-4.7.1
Component: modular forms | Keywords: group, arithmetic, linear
group, sl, modular
Work_issues: amusing typo bug | Upstream: N/A
Reviewer: | Author: Vincent Delecroix
Merged: | Dependencies:
--------------------------------+-------------------------------------------
Changes (by davidloeffler):
* status: needs_review => needs_work
* work_issues: => amusing typo bug
Comment:
Replying to [comment:9 vdelecroix]:
> > Algorithm comments:
> >
> > - For permutation subgroups, almost all calculations are done with a
relabelled version, but this is generated anew every time! I'd suggest:
> >
> > - having a flag that remembers whether the group is already
canonically labelled,
> >
> > - when that's not the case, caching the canonically labelled
version.
>
> Totally right. I choose to add an attribute _canonical_label_group when
the group with canonical renumbering has been computed.
There is an amusingly subtle bug here.
At line 769, the code sets the attribute {{{_canonical_labelel_group}}},
which is clearly a typo. This means that a subsequent hasattr check for
{{{_canonical_label_group}}} returns False, leading to the following:
{{{
sage: S2 = "(1,2)(3,4)(5,6)"
sage: S3 = "(1,3,5)(2,4,6)"
sage: G = ArithmeticSubgroup_Permutation(S2=S2,S3=S3)
sage: H = G.relabel(inplace=False)
sage: G.has_canonical_labels()
False
}}}
I don't think this is what you intended, is it?
The reason I said it's complicated is that naming the method
{{{"has_canonical_labels"}}} is slightly misleading. I would have assumed
that {{{"G.has_canonical_labels() == True"}}} means that the labels G is
using are the canonical ones. Although it's not what you intended, the
typo has meant that the function is now *almost* doing what I feel the
name suggests it should do! ("Almost" because it might return False in
some cases even if the labels actually are the canonical ones.)
I would advocate correcting the typo at line 769 but also changing the
{{{has_canonical_labels}}} function so it does what I said, i.e. so
returns True if and only if {{{self._canonical_rooted_labels() ==
range(self.index())}}}, with a docstring to match.
> > - At line 1019 of {{{arithgroup.py}}}, if check is True, won't this
cause the check code to be run *twice*?
>
> I don't think so.
You're right, of course; sorry.
--
Ticket URL: <http://trac.sagemath.org/sage_trac/ticket/11422#comment:10>
Sage <http://www.sagemath.org>
Sage: Creating a Viable Open Source Alternative to Magma, Maple, Mathematica,
and MATLAB
--
You received this message because you are subscribed to the Google Groups
"sage-trac" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/sage-trac?hl=en.
