sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.45k stars 481 forks source link

FractionWithFactoredDenominator.smooth_critical_ideal should accept names as strings #23640

Open jdemeyer opened 7 years ago

jdemeyer commented 7 years ago

Currently, FractionWithFactoredDenominator.smooth_critical_ideal accepts its variable names as symbolic entries. This is really strange, especially since no symbolic computations are done.

This should be deprecated and the variable names should be accepted as strings (or possibly variables from a pre-existing polynomial ring).

This is needed for #10483.

CC: @simon-king-jena @rwst @dkrenn

Component: asymptotic expansions

Author: Simon King, Ralf Stephan

Branch/Commit: u/jdemeyer/fractionwithfactoreddenominator_smooth_critical_ideal_should_accept_names_as_strings @ acfead5

Reviewer: Daniel Krenn

Issue created by migration from https://trac.sagemath.org/ticket/23640

jdemeyer commented 7 years ago

Branch: u/jdemeyer/fractionwithfactoreddenominator_smooth_critical_ideal_should_accept_names_as_strings

jdemeyer commented 7 years ago

Author: Simon King, Ralf Stephan

jdemeyer commented 7 years ago

Commit: acfead5

jdemeyer commented 7 years ago
comment:2

I'm attaching a patch that I'm moving from #10483. One thing which is missing is the deprecation of the existing behaviour.


New commits:

acfead5Trac #23640: deprecate the misuse of symbolic variables as polynomial variable
jdemeyer commented 7 years ago
comment:5

Minor nitpick about:

            # Coerce alpha into L.
            alpha = [L(a) for a in alpha]

Here you are doing a conversion, not a coercion. Either really do a coercion there or fix the comment.

jdemeyer commented 7 years ago
comment:6

It's also not clear why this coercion/conversion should be done only if indets.

dkrenn commented 7 years ago

Reviewer: Daniel Krenn

dkrenn commented 7 years ago
comment:9

Replying to @jdemeyer:

Minor nitpick about:

            # Coerce alpha into L.
            alpha = [L(a) for a in alpha]

Here you are doing a conversion, not a coercion. Either really do a coercion there or fix the comment.

+1 for using .coerce

dkrenn commented 7 years ago
comment:10

Replying to @jdemeyer:

It's also not clear why this coercion/conversion should be done only if indets.

+1 for doing this always.

dkrenn commented 7 years ago
comment:11
            if isinstance(a, str) and a not in K.variable_names():
                indets.append(a)

There should be an error message, if some a is ignored (i.e., when it is not a string and not an existing variable).