Opened 12 months ago

Closed 8 months ago

#32205 closed enhancement (fixed)

Removing deprecated parameters and methods in projective_ds

Reported by: gh-EnderWannabe Owned by:
Priority: trivial Milestone: sage-9.5
Component: dynamics Keywords: gsoc2021
Cc: bhutz Merged in:
Authors: Alexander Galarraga Reviewers: Ben Hutz, Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 5123115 (Commits, GitHub, GitLab) Commit: 512311586a607902d7e80e873ee95d50ed3ac26a
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

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.

Change History (40)

comment:1 Changed 12 months ago by gh-EnderWannabe

  • Branch set to u/gh-EnderWannabe/remove_deprecations

comment:2 Changed 12 months ago by gh-EnderWannabe

  • Commit set to 90f87989b761e02306b1e86cec4bcb961b46bebf
  • Status changed from new to needs_review

New commits:

90f879832205: removed deprecations

comment:3 Changed 11 months ago by bhutz

  • Reviewers set to Ben Hutz
  • Status changed from needs_review to positive_review

This looks fine to me.

comment:4 Changed 11 months ago by mkoeppe

  • Status changed from positive_review to needs_work

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

comment:5 Changed 11 months ago by bhutz

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

comment:6 Changed 11 months ago by git

  • Commit changed from 90f87989b761e02306b1e86cec4bcb961b46bebf to d905c4814cb4f3c6362e2f29bbcfced677055496

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

795799eMerge branch 'u/gh-EnderWannabe/remove_deprecations' of trac.sagemath.org:sage into deprecations
d905c4832205: throw error when embedding is passed

comment:7 Changed 11 months ago by gh-EnderWannabe

  • Status changed from needs_work to needs_review

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

Last edited 11 months ago by gh-EnderWannabe (previous) (diff)

comment:8 follow-up: Changed 11 months ago by bhutz

  • Status changed from needs_review to needs_work

There error needs to reference the ticket for the deprecation.

comment:9 Changed 11 months ago by git

  • Commit changed from d905c4814cb4f3c6362e2f29bbcfced677055496 to 9e6fd690e8ca385367d74fb249a9d870a8c6c5d1

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

9e6fd6932205: added ticket reference

comment:10 in reply to: ↑ 8 Changed 11 months ago by gh-EnderWannabe

  • Status changed from needs_work to needs_review

Replying to bhutz:

There error needs to reference the ticket for the deprecation.

Added

comment:11 Changed 11 months ago by bhutz

  • Status changed from needs_review to positive_review

comment:12 follow-up: Changed 11 months ago by mkoeppe

  • Status changed from positive_review to needs_work
+        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.

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

comment:14 in reply to: ↑ 13 ; follow-up: Changed 11 months ago by 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.)

Last edited 11 months ago by mkoeppe (previous) (diff)

comment:15 Changed 11 months ago by mkoeppe

  • Description modified (diff)
  • Summary changed from Removing Deprecation Notices in projective_ds to Removing deprecated parameters and methods in projective_ds

comment:16 in reply to: ↑ 12 Changed 11 months ago by 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

comment:17 Changed 11 months ago by mkoeppe

  • Reviewers changed from Ben Hutz to Ben Hutz, Matthias Koeppe

comment:18 in reply to: ↑ 14 ; follow-up: Changed 11 months ago by 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.

comment:19 in reply to: ↑ 18 ; follow-up: Changed 11 months ago by 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.

comment:20 Changed 11 months ago by git

  • Commit changed from 9e6fd690e8ca385367d74fb249a9d870a8c6c5d1 to 3d6dd04ebbb891e194b9c24337bd8336a749611d

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

3d6dd0432205: fixed positional arguments

comment:21 in reply to: ↑ 19 Changed 11 months ago by 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.

comment:22 Changed 11 months ago by gh-EnderWannabe

  • Status changed from needs_work to needs_review

comment:23 Changed 11 months ago by mkoeppe

  • Status changed from needs_review to needs_work

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

comment:24 Changed 11 months ago by git

  • Commit changed from 3d6dd04ebbb891e194b9c24337bd8336a749611d to 36233292cb559a57f73cd2f9caa4f4ab97498fe2

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

362332932205: changed multiplier spectra function definition

comment:25 Changed 11 months ago by gh-EnderWannabe

  • Status changed from needs_work to needs_review

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

comment:27 in reply to: ↑ 26 Changed 11 months ago by 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?

comment:28 follow-up: Changed 11 months ago by 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

comment:29 Changed 11 months ago by git

  • Commit changed from 36233292cb559a57f73cd2f9caa4f4ab97498fe2 to 7a9f9d7c3ae3661d15a0f55b1a9d513d06e03314

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

7a9f9d732205: multiplier spectra fix

comment:30 in reply to: ↑ 28 Changed 11 months ago by 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.

comment:31 Changed 11 months ago by mkoeppe

  • Status changed from needs_review to positive_review

Looking good.

comment:32 Changed 11 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:33 Changed 11 months ago by mkoeppe

  • Branch changed from u/gh-EnderWannabe/remove_deprecations to u/mkoeppe/remove_deprecations

comment:34 Changed 11 months ago by mkoeppe

  • Commit changed from 7a9f9d7c3ae3661d15a0f55b1a9d513d06e03314 to 9a9e4d803feaa70dd30989ed4647b1f13581a5cf

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


New commits:

9a9e4d8Remove deprecated parameters and methods in projective_ds

comment:35 Changed 11 months ago by mkoeppe

  • Status changed from needs_work to positive_review

comment:36 Changed 11 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:37 Changed 8 months ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:38 Changed 8 months ago by git

  • Commit changed from 9a9e4d803feaa70dd30989ed4647b1f13581a5cf to 512311586a607902d7e80e873ee95d50ed3ac26a

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

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

comment:39 Changed 8 months ago by mkoeppe

  • Status changed from needs_work to positive_review

comment:40 Changed 8 months ago by vbraun

  • Branch changed from u/mkoeppe/remove_deprecations to 512311586a607902d7e80e873ee95d50ed3ac26a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.