#13215: Skew polynomials
-------------------------------------+-------------------------------------
Reporter: caruso | Owner: tbd
Type: enhancement | Status: needs_work
Priority: major | Milestone: sage-7.3
Component: algebra | Resolution:
Keywords: skew polynomials | Merged in:
Authors: Xavier Caruso | Reviewers: Burcin Erocal
Report Upstream: N/A | Work issues: see comment #25
Branch: | Commit:
u/arpitdm/skew_polynomials | e189fec13d005a7fba39a429876c501fe95c05da
Dependencies: #13214, #13303, | Stopgaps:
#13640, #13641, #13642 |
-------------------------------------+-------------------------------------
Changes (by dlucas):
* status: needs_review => needs_work
Comment:
Hello,
I started reviewing your work. For now, I mostly read documentation and
checked
formatting, without focusing on the code itself.
Here are my comments:
== Generic comments:
- For now, there's nothing related to skew polynomials in the index file,
so it won't be possible
for the user to access the documentation through the online reference
manual.
Can you please add a dedicated section in the related index file? See
`src/doc/en/reference/polynomial_rings/index.rst`.
- I would also use the experimental decorator here: the code in itself
seems stable, but it
will allow us to make later name changes (for instance) without putting
deprecation warnings
all over the place. In particular, I have in mind later implementations
of sparse rings and
multivariate rings, which might trigger some renaming in the classes
implemented here.
== skew_polynomial_ring_constructor.py
- I might be wrong, but it does not seem to have any reason to get both
input `name` and `names`.
The argument in the `NOTE` block sounds weird to me, especially when
reading the code: it seems
to be some kind of argument for later enhancements (multivariate skew
polynomial rings), as for now
it does not work if the list contains more than one element. Furthermore,
the input `names` is
only used to set `name` if the latter is `None`, and dropped just after.
And in any case, I think it's better to use only one input and use type
checks.
I propose to remove `names`, rename `name` into `variable_names`, and use
sanity checks: if it's
a string, or a list with a single string, use it, otherwise return an
appropriate error.
I also suggest to add a line in the note block to say that multivariate
rings are not
supported (it's only detailed in tests for now).
- Can you please explicit what `LHS`/`RHS` means?
- There's an option `sparse=False` which, if set to `True`, returns a
`NotImplementedError`.
I suggest to remove it for now in order to not confuse the user, and
reinstate it later
when it will be fully implemented. In this case, also remove all
references to this feature
in the documentation.
- `INPUT` block could be enhanced: I would rather explain what's `name`
(or `variable_names` if
you agree with my comment above) instead of just saying "a string". I
would also rename `sigma`
to something more indicative. What about `base_ring_endomorphism`?
- As I suggest to remove some not implemented but planned features, it
might be a good
idea to add a `TODO` block as a reminder of these features (sparse rings,
multivariate rings).
- Module title says "constructorS for skew polynomial rings", while
there's only one method
in this module. `</quibbling>`
- Module documentation itself is not great. I think it might be a good
idea to add a few
links to `skew_polynomial_ring.py`, and even to give a small definition
of a skew polynomial ring
in this module.
== skew_polynomial_ring.py
- The short description of this module:
"Sage implements dense skew univariate polynomials over commutative
rings.",
seems unhelpful. I'd rather replace it by something explaining the
purpose of this
module.
- I think the definition can be enhanced a bit: for instance, you find
references to "twisting"
all over the examples, but it's not defined everywhere. Reading what's in
the `DEFINITION` block
should be enough to understand the terminology used in the module, even
if you're not familiar with
skew polynomials at all.
- Same remark as above regarding `LHS`/`RHS`.
- Some imports are not necessary anymore, e.g. `import sage.misc.latex as
latex`.
- Some methods here do not follow the mandatory documentation template.
For instance, `__init__` does not have a short description, nor an
`INPUT` block...
- There are a few methods in this file which are commented. If these are
no longer useful,
I suggest to remove them instead of commeting them.
- In some methods, documentation is not really helpful: see `twist_map`
for instance.
If says "Return the twist map [...]" which is not very informative...
- While I'm talking about it, would it be possible to explain why this
method might fail
if you pass a negative number as input? The sentence "Sometimes it
succeeds" seems to imply
it's more or less random, but if you could investigate this a bit it
would lead to a more
satisfying documentation.
== skew_polynomial_element.py
- Same remark as above regarding the definition of a skew polynomial ring.
- Same remark as above regarding imported packages.
- Same remark as above regarding informativeness of docstrings for some
methods.
- Same remark as above regarding documentation itself: a few methods (e.g.
`conjugate`) do
not have a short description, while `make_generic_skew_polynomial` does
not have documentation at all!
Best,
David
--
Ticket URL: <https://trac.sagemath.org/ticket/13215#comment:59>
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 https://groups.google.com/group/sage-trac.
For more options, visit https://groups.google.com/d/optout.
