Removing deprecated parameters and methods in projective_ds

[gh-EnderWannabe]

Currently there are several old deprecation warnings in dynamical systems on projective space. They are:

• deprecation of the keyword embedding in sigma_invariants (4 years old, #23333)

• deprecation of rational_periodic_points method (2 years old, #28109)

• deprecation of rational_preperiodic_points method (2 years old, #28213)

We remove these deprecated methods in this ticket, and make it an error if the deprecated keyword is passed.

[gh-EnderWannabe]

New commits:

​90f8798

32205: removed deprecations

[bhutz]

This looks fine to me.

[mkoeppe]

I don't think this is the correct next step after deprecating the embedding keyword. It should become an error if embedding is passed.

[bhutz]

if that's the process, then ok. But note that the keyword hasn't been accepted for 4 years now.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​795799e	Merge branch 'u/gh-EnderWannabe/remove_deprecations' of trac.sagemath.org:sage into deprecations
​d905c48	32205: throw error when embedding is passed

[gh-EnderWannabe]

Ok, added an error when the embedding keyword is passed.

[bhutz]

There error needs to reference the ticket for the deprecation.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​9e6fd69

32205: added ticket reference

[gh-EnderWannabe]

Replying to bhutz:

There error needs to reference the ticket for the deprecation.

Added

[mkoeppe]

+        if not embedding is None:
+            raise ValueError('the embedding keyword is deprecated, see :trac:`32205`')

this makes no sense - "deprecated" means that it still works

Also note that changes like this:

-    def sigma_invariants(self, n, formal=False, embedding=None, type='point'):
+    def sigma_invariants(self, n, **kwds):

make an incompatible change to the signature -- after the change, it is not possible any more to pass these arguments as positional arguments.

This is something to keep in mind when creating new code -- if arguments are meant to be keyword-only, they should be marked as such.

Also a comment on the ticket description:

We remove these old deprecations in this ticket.

One does not "remove deprecations" -- one removes deprecated features or deprecated code.

[bhutz]

This is getting a little circular now.

The signature is changed because you asked for the embedding keyword to be accepted so that an error could be raised. This particular function has never accepted the embedding keyword. The function it replaced was deprecated 2 years ago. That replaced function accepted an embedding keyword until 4 years ago. So adding an embedding keyword to this function just to raise an error does not seem like a good choice.

What should have happened is that after the embedding keyword was deprecated that change was followed-up on before the whole function was deprecated and replaced. However, that's not where we are today.

So, should Alex continue and adjust the error message to be better phrased and leave the code as is or go back to the original solution which seemed to most logical based on the current situation?

Since you have strong opinions about this particular issue, I'm happy to back off and let you handle the review.

[mkoeppe]

Replying to bhutz:

you asked for the embedding keyword to be accepted so that an error could be raised.

No, actually ​90f8798 had kept the parameter and removed the warning and the documentation of the parameter - so effectively the function would silently accept this parameter.

I asked for it to become an error to pass the previously deprecated parameter.

Normally one would just remove the parameter from the function signature. Then it becomes an error to pass the parameter just by the fact that passing an unknown keyword argument is an error in Python.

However, you have the problem that these functions also accept these arguments as positional arguments. By just removing the embedding parameter, you are changing the signature -- a previous call of .sigma_invariants(n, False, None, 'point') would no longer be valid.

The current change, to **kwds, makes all calls like .sigma_invariants(n, False) invalid too. This is an incompatible change and should not be done without deprecation.

(See #16607 for a related discussion.)

[mkoeppe]

Replying to mkoeppe:

Also a comment on the ticket description:

We remove these old deprecations in this ticket.

One does not "remove deprecations" -- one removes deprecated features or deprecated code.

Fixed it for you in description and title

[gh-EnderWannabe]

Replying to mkoeppe:

However, you have the problem that these functions also accept these arguments as positional arguments. By just removing the embedding parameter, you are changing the signature -- a previous call of .sigma_invariants(n, False, None, 'point') would no longer be valid.

This is an excellent point I had not thought about.

If we change the code to throw an error when an embedding parameter is passed, then we have reached a dead end where sigma_invariants has a useless parameter which must be documented as useless, indefinitely. This seems sub optimal, and confusing for a user.

Is there some way to avoid having to maintain a useless parameter? I can't think of one short of deprecating the entire function and creating a new function with a new name but identical functionality.

If there isn't a way to avoid maintaining a useless parameter, I'm inclined to leave embedding as deprecated. That way it is at least clear why embedding is still accepted, and if we add a note referencing this ticket, hopefully no one will make my same mistake.

[mkoeppe]

Replying to gh-EnderWannabe:

If we change the code to throw an error when an embedding parameter is passed, then we have reached a dead end where sigma_invariants has a useless parameter which must be documented as useless, indefinitely. This seems sub optimal, and confusing for a user.

Yes, it is an unfortunate situation that comes from the fact that Sage was stuck in Python 2 for too long, in which the syntax for marking keyword-only parameters was not available. It is affecting many interfaces in Sage.

Is there some way to avoid having to maintain a useless parameter?

I'm not aware of a satisfactory mechanism really. We have a @rename_keyword decorator, but (as far as I know) not a decorator that would make it an error to pass the keyword - without changing the positional signature.

I can't think of one short of deprecating the entire function and creating a new function with a new name but identical functionality.

Well, a possible way forward would be to decide to deprecate passing certain parameters as positional arguments. In the example of sigma_invariants, I would guess that you would be OK to make all arguments except n keyword-only at some point in the future.

So along the lines of the discussion in #16607, one could introduce this deprecation. Unfortunately this may lead to rather complicated code. So ideally it would be implemented using a decorator instead of doing it one by one in every function that needs it.

After that deprecation has expired, one would finally be able to remove the parameter completely.

If there isn't a way to avoid maintaining a useless parameter, I'm inclined to leave embedding as deprecated. That way it is at least clear why embedding is still accepted, and if we add a note referencing this ticket, hopefully no one will make my same mistake.

Yes, I think this is a simple reasonable solution; but instead of calling deprecation, you could as well raise an error.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​3d6dd04

32205: fixed positional arguments

[gh-EnderWannabe]

Replying to mkoeppe:

Yes, I think this is a simple reasonable solution; but instead of calling deprecation, you could as well raise an error.

Ok, pushed a branch with such an error.

[mkoeppe]

Looking good, but multiplier_spectra will need a fix for the positional arguments as well

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​3623329

32205: changed multiplier spectra function definition

[mkoeppe]

--- a/src/sage/dynamics/arithmetic_dynamics/projective_ds.py
+++ b/src/sage/dynamics/arithmetic_dynamics/projective_ds.py
@@ -4149,7 +4149,7 @@ class DynamicalSystem_projective(SchemeMorphism_polynomial_projective_space,
         else:
             raise ValueError("algorithm must be either 'variety' or 'cyclegraph'")
 
-    def multiplier_spectra(self, n, formal=False, type='point', use_algebraic_closure=True):
+    def multiplier_spectra(self, n, formal=False, embedding=None, type='point'):
         r"""
         Computes the ``n`` multiplier spectra of this dynamical system.
 

I don't understand what the purpose of this change is

@@ -4172,6 +4172,8 @@ class DynamicalSystem_projective(SchemeMorphism_polynomial_projective_space,
           to find the formal ``n`` multiplier spectra of this map and
           ``False`` specifies to find the ``n`` multiplier spectra
 
+        - ``embedding`` -- (default: ``None``) ignored. See :trac: `32205`.
+
         - ``type`` -- (default: ``'point'``) string; either ``'point'``
           or ``'cycle'`` depending on whether you compute one multiplier
           per point or one per cycle
@@ -4296,6 +4298,9 @@ class DynamicalSystem_projective(SchemeMorphism_polynomial_projective_space,
         PS = self.domain()
         n = Integer(n)
 
+        if not embedding is None:
+            raise ValueError('do not specify an embedding')
+
         if (n < 1):
             raise ValueError("period must be a positive integer")
         if not is_ProjectiveSpace(PS):

Documentation does not match the implementation. embedding is not ignored. How about something like this:

>>>        - ``embedding`` -- (default: ``None``) must be ``None``, passing an embedding is no longer supported, see :trac:`32205`.

Likewise in the other method.

[gh-EnderWannabe]

Replying to mkoeppe:

I don't understand what the purpose of this change is

Sorry, I thoughtlessly changed multiplier spectra. You said that multiplier spectra will need a fix for positional arguments as well. Looking back at commits, I don't think multiplier spectra was accepting a deprecated argument before this ticket. Could you clarify?

[mkoeppe]

For this method, I think you only need to undo the change to using **kwds that you did in ​https://git.sagemath.org/sage.git/commit/?id=d905c4814cb4f3c6362e2f29bbcfced677055496

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​7a9f9d7

32205: multiplier spectra fix

[gh-EnderWannabe]

Replying to mkoeppe:

For this method, I think you only need to undo the change to using **kwds that you did in ​https://git.sagemath.org/sage.git/commit/?id=d905c4814cb4f3c6362e2f29bbcfced677055496

Thanks for clarifying. Hopefully that last push does the trick.

[mkoeppe]

Looking good.

[vbraun]

Merge conflict

[mkoeppe]

Squashed/rebased on 9.4.rc0 to remove the merge conflict

---

New commits:

​9a9e4d8

Remove deprecated parameters and methods in projective_ds

[vbraun]

Merge conflict

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​5123115

Merge tag '9.5.beta4' into t/32205/remove_deprecations