#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:
Branch: | Commit:
u/arpitdm/skew_polynomials | e189fec13d005a7fba39a429876c501fe95c05da
Dependencies: #13214, #13303, | Stopgaps:
#13640, #13641, #13642 |
-------------------------------------+-------------------------------------
Comment (by arpitdm):
Replying to [comment:59 dlucas]:
> - 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 read up a little on Sphinx documentation and from what I understand, I
need to add the following in the file you mentioned, right?
{{{
@sage.misc.superseded.experimental(trac_number=13215)
Skew Polynomials
----------------
.. toctree::
:maxdepth: 2
sage/rings/polynomial/skew_polynomial_element
sage/rings/polynomial/skew_polynomial_ring_constructor
sage/rings/polynomial/skew_polynomial_ring
}}}
> - 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.
Placing the experimental warning (like I did above) is not the right way
to do it because it does not throw an error. Do I instead place this line
at the start of every class in all the three files instead?
> == 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).
I agree. `names` and `name` are both not required and `variable_names`
along with tests to check type of the input is cleaner. I've made this
change.
> - Can you please explicit what `LHS`/`RHS` means?
Like @tscrim mentions, I think LHS/RHS are extremely common terms that are
used almost everywhere and perhaps don't need explicit description.
> - 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.
There are a lot of places where a NotImplementedError is raised and not
just in this ticket but in other polynomial files as well. I think it
makes more sense to have them if they are already there. However, there is
then the inconsistency in that derivations in skew polynomials are not
implemented and are marked as `todo` as is the multivariate case in some
places. Still, I argue for keeping sparse as it is since it is integrated
in the framework already unlike the other two. And with the experimental
warning showing up, changing this will not create too much trouble.
> - `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`?
An endomorphism that admits an inverse is an automorphism and skew
polynomial rings AFAIK are based on automorphisms. Please correct me if
I'm mistaken in this. If so, the places where it says that the twist map
is not invertible, maybe should say that twist map should be invertible.
And accordingly, the name can be changed to `base_ring_automorphism`.
> - 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.
Understood. I've made changes and expanded on the definitions and examples
from other files. Same goes for all other notes (below) about improving
explanations.
> - Some imports are not necessary anymore, e.g. `import sage.misc.latex
as latex`.
I tried to do that with the ones I thought were not needed/deprecated.
I've obviously missed a few. I'll go over this once again.
> - Some methods here do not follow the mandatory documentation template.
> For instance, `__init__` does not have a short description, nor an
`INPUT` block...
Is there a reference doc or something where I can go through the mandatory
documentation template? Or is it that Description, Input, Output,
Examples, Tests - should these be present for every method? A lot of
places, it doesn't make sense to have all of this.
> - In some methods, documentation is not really helpful: see `twist_map`
for instance.
> not have a short description, while `make_generic_skew_polynomial` does
not have documentation at all!
Oh right! I mentioned this in the email thread with Johan except that I
somehow messed up the name. I wasn't sure of what this method was doing.
--
Ticket URL: <https://trac.sagemath.org/ticket/13215#comment:62>
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.
