#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 |
-------------------------------------+-------------------------------------
Comment (by tscrim):
Replying to [comment:59 dlucas]:
> - 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.
It is okay to change class names as long as the public API doesn't change
(although this can entail unpickling issues in either case). I would
instead future-proof this by
> - Can you please explicit what `LHS`/`RHS` means?
I think these are such common abbreviations for Left(Right)-Hand Side that
being explicit about them will add some clutter.
> - 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.
> - 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).
I disagree with these comments. This is documenting an upcoming feature,
it helps keeps the API consistent with future plans and other parts of
Sage, and tells someone reading the code what is currently not
implemented.
> - Some methods here do not follow the mandatory documentation template.
> For instance, `__init__` does not have a short description, nor an
`INPUT` block...
I treat this more as a strong guideline than a hard rule, but with that
being said, anything more non-trivial should at least document its inputs
(e.g., the `__init__` would be useful).
I agree with the rest of David's comments.
Also, from a cursory glance, there seems to be a lot of duplication with
the current implementations of polynomials (i.e., the elements). I feel
there could be a lot of simplification of the code by either subclassing
or creating a mix-in (cython) class that overrides the `_mul_` of
polynomials. Is there a reason why you didn't do this? This might entail
doing some refactoring/abstracting of the polynomial code though, but it
should make the implementation of skew polynomials a lot easier (and bug
proof).
--
Ticket URL: <https://trac.sagemath.org/ticket/13215#comment:60>
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.
