#16014: Improvements to discriminant computation
-------------------------------------+-------------------------------------
Reporter: gagern | Owner:
Type: defect | Status: needs_review
Priority: major | Milestone: sage-6.2
Component: algebra | Resolution:
Keywords: discriminant | Merged in:
Authors: | Reviewers: Peter Bruin
Report Upstream: N/A | Work issues:
Branch: | Commit:
u/gagern/ticket/16014 | 1892c84d6372e882fa0c39667d06ba83cc61f3be
Dependencies: | Stopgaps:
-------------------------------------+-------------------------------------
Changes (by pbruin):
* reviewer: => Peter Bruin
Comment:
Replying to [comment:7 gagern]:
> Replying to [comment:5 pbruin]:
> > I'm wondering if you couldn't be convinced to adopt John Cremona's
[https://groups.google.com/d/msg/sage-support/fABfhHyioa0/kPlhGN4Ri-EJ
proposal on sage-dev] to have a separate function
`generic_univariate_discriminant()` (or similar), which caches its output,
and hardcodes it for small degrees.
>
> Sure I can be convinced, particularly if two people request something.
I've implemented the caching, but not the hardcoding so far. I wonder how
small “small” actually should be. Up to 7, things compute fairly quickly
so there seems to be little benefit from hardcoding, particularly since we
do caching.
Thanks, this looks very good. You're probably right about not hardcoding
the polynomials.
> For higher degrees, computing them for hardcoding takes considerable
time, and writing down the hard code will take up quite a lot of space in
the source file. Should this really be implemented in Cython code, or
should we make some effort to obtain or compute discriminants up to say 10
or so and store them in some kind of persistent data representation
instead of source code?
Maybe, but I doubt it is very important. If at all, it should be done in
a later ticket.
> I also wonder how public this function should be. I prefer
`polynomial_element` over `invariant_theory` simply because it keeps all
my changes in one place. I've written a full docstring for the code, and
inside that docstring I had to import the function. Is this a reasonable
approach? Should I add the function to `sage.rings.polynomial.all`? Should
I prepend the function name with an underscore to mark it non-public?
I actually agree completely with the way you did all these things. I
wouldn't add anything to `sage.rings.polynomial.all`; this would add it to
the global namespace, which is already very crowded.
> I also have a question regarding my late import. I noticed the `cdef
void late_import` near the beginning of the module. I could add
`is_PowerSeriesRing` to that. But on the other hand, it seems as if that
function only gets called by `Polynomial.roots`. Which suggests that I
might not be able to rely on `is_PowertSeriesRing` if I were to follow
that approach. Do you agree with keeping the import where I have it so
far? Should the function `late_import` be renamed to something like
`late_import_for_root` to make it clearer that this does no general late
importing but specific to that method? Or should I add my imports to that
function and make sure to call `late_import` in `discriminant` as well?
Here, too, I think the way you did it is absolutely fine. In fact one can
argue that modules should generally be imported locally in the function
that needs it. On the other hand, global imports (including the
`late_import` approach) are useful if the imported module is used in many
places, or if the function is time-critical enough that a local import
would add too much overhead.
> Should I add myself as an author of that module, at the beginning of the
file?
If you want; as you will have seen, authors for some other
additions/changes are listed, but it is certainly not done in a very
systematic way througout Sage. In any case, your changes and authorship
will be remembered by the revision control system, and using `git
annotate` gives a line-by-line overview of who wrote/changed what.
Could you also fill in your name (i.e. real name, not Trac username) in
the "Author" field of this ticket?
I will test this and then set it to positive review.
--
Ticket URL: <http://trac.sagemath.org/ticket/16014#comment:8>
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.
