Opened 2 years ago

Last modified 2 years 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 2 years ago by jdemeyer

  • Branch set to u/jdemeyer/fractionwithfactoreddenominator_smooth_critical_ideal_should_accept_names_as_strings

comment:2 Changed 2 years 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 2 years ago by jdemeyer

  • Status changed from new to needs_review

comment:4 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:5 follow-up: Changed 2 years 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 2 years ago by jdemeyer

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

Last edited 2 years ago by jdemeyer (previous) (diff)

comment:7 Changed 2 years ago by jdemeyer

  • Cc dkrenn added

comment:8 Changed 2 years ago by dkrenn

  • Reviewers set to Daniel Krenn

comment:9 in reply to: ↑ 5 Changed 2 years 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 2 years 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 2 years 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.