#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 dlucas):
Replying to [comment:62 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
> }}}
Yup! I you're not sure of what you're doing, here's a way to test it:
run `sage --docbuild src/doc/en/reference/polynomial_rings html`, it will
compile the
doc and give you an address. Copy-paste this address in your browser, and
click on "index"
in the page which opens. If what you added in `index.rst` works, you
should see it here (and
you'll be able to check its formatting as well).
\\\\
> > - 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?
Mmh, I reconsidered my position on this thanks to Travis' comment: as
we'll not change the public API (which is the method in
`skew_polynomial_ring_constructor.py`), I don't think it's necessary to
put experimental warnings everywhere, as long as we're future proof as
Travis suggests.
\\\\
> > - 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.
Yes, you're both right, let's drop that and keep `LHS`/`RHS`.
\\\\
> > - 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.
Yes, thinking again about it, I think you're both right. It's better to be
future proof, and if
it's used elsewhere in `polynomials`, there's definitely no trouble in
doing so.
\\\\
> > - `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`.
Err, yes my bad. `base_ring_automorphism` sounds good `:)`
\\\\
> > - 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.
Don't worry, it's quite tricky.
\\\\
> > - 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.
You can read this
[http://doc.sagemath.org/html/en/developer/coding_basics.html
#documentation-strings tutorial] for Sage developers.
But I agree this is not strong rules: while I do follow the "short
description, jump line, optional
long description" for my methods, I only write an `OUTPUT` block when the
output is not obvious from reading the short description. I always write
an `INPUT` block when there's input arguments, as I
think that even if the names of the input arguments seems obvious, it's
always nice to have an
explicit definition. `EXAMPLES` are of course mandatory. `TESTS` are not,
I think you should only
use them when you want to run a few more tests which do not really make
sense for the user. For instance, I use `TESTS` to doctest sanity checks.
It's nice to add a doctest to ensure everything is
sanitized, but it's of little interest for the user.
\\\\
> > - 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.
Oh ok! I'll read it and try to understand what it does.
Best,
David
--
Ticket URL: <https://trac.sagemath.org/ticket/13215#comment:63>
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.
