Opened 5 years ago

Last modified 5 years ago

#23640 needs_work defect

FractionWithFactoredDenominator.smooth_critical_ideal should accept names as strings

Reported by: Jeroen Demeyer Owned by:
Priority: major Milestone: sage-8.1
Component: asymptotic expansions Keywords:
Cc: Simon King, Ralf Stephan, Daniel Krenn Merged in:
Authors: Simon King, Ralf Stephan Reviewers: Daniel Krenn
Report Upstream: N/A Work issues:
Branch: u/jdemeyer/fractionwithfactoreddenominator_smooth_critical_ideal_should_accept_names_as_strings (Commits, GitHub, GitLab) Commit: acfead5331c4e2c95d974ec909885ebba9642c8e
Dependencies: Stopgaps:

Status badges

Description

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.

Change History (11)

comment:1 Changed 5 years ago by Jeroen Demeyer

Branch: u/jdemeyer/fractionwithfactoreddenominator_smooth_critical_ideal_should_accept_names_as_strings

comment:2 Changed 5 years ago by Jeroen Demeyer

Authors: Simon King, Ralf Stephan
Cc: Simon King Ralf Stephan added
Commit: acfead5331c4e2c95d974ec909885ebba9642c8e

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

comment:3 Changed 5 years ago by Jeroen Demeyer

Status: newneeds_review

comment:4 Changed 5 years ago by Jeroen Demeyer

Status: needs_reviewneeds_work

comment:5 Changed 5 years ago by Jeroen Demeyer

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.

comment:6 Changed 5 years ago by Jeroen Demeyer

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

Last edited 5 years ago by Jeroen Demeyer (previous) (diff)

comment:7 Changed 5 years ago by Jeroen Demeyer

Cc: Daniel Krenn added

comment:8 Changed 5 years ago by Daniel Krenn

Reviewers: Daniel Krenn

comment:9 in reply to:  5 Changed 5 years ago by Daniel Krenn

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

comment:10 in reply to:  6 Changed 5 years ago by Daniel Krenn

Replying to jdemeyer:

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

+1 for doing this always.

comment:11 Changed 5 years ago by Daniel Krenn

            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).

Note: See TracTickets for help on using tickets.