Opened 22 months ago

Last modified 22 months ago

#23640 needs_work defect

FractionWithFactoredDenominator.smooth_critical_ideal should accept names as strings

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.1
Component: asymptotic expansions Keywords:
Cc: SimonKing, rws, dkrenn 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) Commit: acfead5331c4e2c95d974ec909885ebba9642c8e
Dependencies: Stopgaps:

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 22 months ago by jdemeyer

  • Branch set to u/jdemeyer/fractionwithfactoreddenominator_smooth_critical_ideal_should_accept_names_as_strings

comment:2 Changed 22 months ago by jdemeyer

  • Authors set to Simon King, Ralf Stephan
  • Cc SimonKing rws added
  • Commit set to 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 22 months ago by jdemeyer

  • Status changed from new to needs_review

comment:4 Changed 22 months ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:5 follow-up: Changed 22 months ago by 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.

comment:6 follow-up: Changed 22 months ago by jdemeyer

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

Last edited 22 months ago by jdemeyer (previous) (diff)

comment:7 Changed 22 months ago by jdemeyer

  • Cc dkrenn added

comment:8 Changed 22 months ago by dkrenn

  • Reviewers set to Daniel Krenn

comment:9 in reply to: ↑ 5 Changed 22 months ago by dkrenn

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 22 months ago by dkrenn

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 22 months ago by dkrenn

            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.