#17792: Word problem for FareySymbol
-------------------------------------+-------------------------------------
Reporter: mmasdeu | Owner:
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-6.7
Component: modular forms | Resolution:
Keywords: farey symbol, | Merged in:
SL2Z, word problem | Reviewers:
Authors: Marc Masdeu | Work issues:
Report Upstream: N/A | Commit:
Branch: u/mmasdeu/17792 | 3de10d68173dc99937c52af0cc5df5b8e2b2017a
-farey-wordproblem | Stopgaps:
Dependencies: |
-------------------------------------+-------------------------------------
Changes (by vdelecroix):
* status: needs_review => needs_work
Comment:
Hello,
When you construct a tuple, there is no need to construct a list first!
You can replace
{{{
tuple([X for Y in Z])
}}}
by
{{{
tuple(X for Y in Z)
}}}
I actually do not like so much the fact the three methods do actually
exactly the same job and are called differently... Why not adding an
argument to `tietze` or `word_problem` to choose the output type? Can be
- `"standard"`: just with `1,2,3,...` and `-1,-2,-3,...` as it was (the
current `tietze`)
- `"gens_index"`: as pairs `(i,+1)` or `(i,-1)` where `i` is the
generator (the current `syllabes`)
- `"gens"`: as pairs `(m,+1)` or `(m,-1)` where `m` is one of the
generator (the current `word_problem).
In the case of `word_problem` above wouldn't it be better to output a list
of matrices (that can be either a generator or inverse of a generator)?
The public method `get_pmats_to_gens` is not appropriate if it has to be
public. The names must be complete: `paring_matrices_to_gens_table` or
something similar. Again, why is this method `1`-based if there `gens` in
the name? Call it `pairing_matrices_to_tietze_index` but do not mention
`gens` if it has to be `1`-based.
Still in `get_pmats_to_gens`, why not using `cached_method` if you need to
cache the result? By the way, is there really a need to make it cached?
Still in `get_pmats_to_gens`, this `try/except` blocks are ugly. Moreover,
doing `l.index` is a linear search through a list. So I would rather do
{{{
gens_dict = {g:i for i,g in enumerate(self.generators())}
ans = []
for pm in self.pairing_matrices():
if pm in gens_dict:
ans.append(gens_dict[pm])
continue
m = -pm
if m in gens_dict:
ans.append(gens_dict[m])
continue
m = ~pm
if m in gens_dict:
ans.append(-gens_dict[m])
continue
m = -m
if m in gens_dict:
ans.append(-gens_dict[m])
continue
raise RuntimeError("this should not happen, right?")
return ans
}}}
Vincent
--
Ticket URL: <http://trac.sagemath.org/ticket/17792#comment:19>
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 unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
Visit this group at http://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
