Opened 6 years ago
Closed 5 years ago
#16023 closed task (fixed)
remove deprecated code in functions/ and symbolic/
Reported by:  rws  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  symbolics  Keywords:  deprecation 
Cc:  Merged in:  
Authors:  Ralf Stephan  Reviewers:  Burcin Erocal, KarlDieter Crisman 
Report Upstream:  N/A  Work issues:  
Branch:  633a411 (Commits)  Commit:  633a4117fee037d40cb18adfc281d3451fad77dc 
Dependencies:  Stopgaps: 
Description
Change History (34)
comment:1 Changed 6 years ago by
 Branch set to u/rws/ticket/16023
 Commit set to 0b379e829059b3c95faa21176e46c75473baff96
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Reviewers set to Burcin Erocal
 Status changed from needs_review to needs_work
Many thanks for cleaning up!
It looks like some function bodies are reduced to BuiltinFunction.__call__()
or GinacFunction.__call__()
. Moving the doctest somewhere else and removing these functions would be better.
comment:3 Changed 6 years ago by
 Commit changed from 0b379e829059b3c95faa21176e46c75473baff96 to eb0fcadcb6adeab9d97d58ab1db45ad340c1b76e
Branch pushed to git repo; I updated commit sha1. New commits:
eb0fcad  remove superfluous __call__ funcs

comment:4 Changed 6 years ago by
 Status changed from needs_work to needs_review
Thanks. I believe you mean Ei
and exp
.
comment:5 followup: ↓ 7 Changed 6 years ago by
Looks good to me. You can switch to positive review once the patchbot gives it a greenlight. (Does patchbot work with the git workflow now?)
comment:6 Changed 6 years ago by
 Commit changed from eb0fcadcb6adeab9d97d58ab1db45ad340c1b76e to 4a9b2e19851e68bae5106f1ae8e27876b107de69
Branch pushed to git repo; I updated commit sha1. New commits:
4a9b2e1  fix typo

comment:7 in reply to: ↑ 5 Changed 6 years ago by
Replying to burcin:
Looks good to me. You can switch to positive review once the patchbot gives it a greenlight. (Does patchbot work with the git workflow now?)
Not here. This is still unresolved: https://github.com/robertwb/sagepatchbot/issues/10
Thanks.
comment:8 followup: ↓ 13 Changed 6 years ago by
The Bessel functions weren't removed that long ago, and note that there is a LOT more to be removed  namely, the whole underscore versions of them, see http://git.sagemath.org/sage.git/tree/src/sage/functions/bessel.py?id=9db8c5c598ec9de953a33b14e531bee3c092c199#n1102  basically to the end of the document.
Still, hopefully it wouldn't be a problem. I am pretty sure that all the doctests in the stuff to be removed is replicated elsewhere, but it might be worth checking out just to be sure. Also note that here there is a small bit to be removed. I wouldn't cry if the whole Bessel removal were another ticket.
comment:9 Changed 6 years ago by
 Branch changed from u/rws/ticket/16023 to u/rws/ticket/160231
 Commit changed from 4a9b2e19851e68bae5106f1ae8e27876b107de69 to e964cf5f5a6553ba90aac03dd1e7822d4e5dcece
Last 10 new commits:
ba8a5a6  ref doc completed; coefficients(), slicing added

be9bb7f  handle ogf > 1

b39485b  move to rings/, allow 1/x, subclass from FractionFieldElement

cc2b400  add, sub functions

998ed74  mul, div

ca22f6d  be specific about base ring, more doctests

8b74445  specify applicability, handle constants, more doctests, better documentation

a9a19a9  use pari script default for guessing; safeguards in ctor

662a41b  last doctests, fix documentation

e964cf5  Merge branch 'u/rws/ticket/16023' of trac.sagemath.org:sage into tmp

comment:10 Changed 6 years ago by
 Branch changed from u/rws/ticket/160231 to u/rws/ticket/16023
 Commit changed from e964cf5f5a6553ba90aac03dd1e7822d4e5dcece to 4a9b2e19851e68bae5106f1ae8e27876b107de69
Oops wrong branch.
comment:11 Changed 6 years ago by
Now the branch doesn't seem to be coming up...
comment:12 Changed 6 years ago by
 Commit changed from 4a9b2e19851e68bae5106f1ae8e27876b107de69 to 2b232633fbdc27c73f3eb77163217bbeda773338
comment:13 in reply to: ↑ 8 ; followup: ↓ 14 Changed 6 years ago by
Replying to kcrisman:
The Bessel functions weren't removed that long ago, and note that there is a LOT more to be removed  namely, the whole underscore versions of them, see http://git.sagemath.org/sage.git/tree/src/sage/functions/bessel.py?id=9db8c5c598ec9de953a33b14e531bee3c092c199#n1102  basically to the end of the document.
This is now #16173
... Also note that here there is a small bit to be removed.
Done.
comment:14 in reply to: ↑ 13 Changed 6 years ago by
I'm really sorry to be nitpicky, this is actually really good to do.
The Bessel functions weren't removed that long ago, and note that there is a LOT more to be removed  namely, the whole underscore versions of them, see http://git.sagemath.org/sage.git/tree/src/sage/functions/bessel.py?id=9db8c5c598ec9de953a33b14e531bee3c092c199#n1102  basically to the end of the document.
This is now #16173
Do you want to wait on removing the deprecated Bessel things to there, then? I guess that's my point. (But not the little bit below about prec
, that is okay to remove here.)
... Also note that here there is a small bit to be removed.
Done.
And if you can still keep a doctest which does something like gamma(6.)
that would be useful, as it seems to sometimes unlock inconsistencies on different platforms we otherwise might not know about.
sage: search_src("gamma\(6") functions/other.py:758: sage: gamma(6, prec=53) functions/other.py:820: sage: log_gamma(6) functions/other.py:822: sage: log_gamma(6.) functions/other.py:824: sage: log_gamma(6).n() functions/other.py:997: sage: gamma(6) rings/rational.pyx:2920: sage: gamma(6/1)
The following test ought to do it.
sage: gamma(6.) 120.000000000000
comment:15 Changed 6 years ago by
 Commit changed from 2b232633fbdc27c73f3eb77163217bbeda773338 to 1946f517f92292231eb244ad2033dbe16e755244
Branch pushed to git repo; I updated commit sha1. New commits:
1946f51  Merge branch 'develop' into t/16023/ticket/16023

comment:16 Changed 6 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:17 Changed 6 years ago by
 Commit changed from 1946f517f92292231eb244ad2033dbe16e755244 to b856623e25d8261d7442433a0b1fafd13b8cc1bf
comment:18 Changed 6 years ago by
 Commit changed from b856623e25d8261d7442433a0b1fafd13b8cc1bf to 1a00c65b8ee20bc16c9e36e0e1936f56fc8c86b9
Branch pushed to git repo; I updated commit sha1. New commits:
1a00c65  Merge branch 'develop' into t/16023/ticket/16023

comment:19 Changed 6 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:20 followup: ↓ 24 Changed 6 years ago by
Somewhat surprisingly, given the attention to detail you've clearly given, there is still work to be done here  but easy work, I believe.
 In other.py, in the
__call__
method for gamma, there is still an import of deprecation which is now not needed.  This comment suggests that perhaps
sage: Q.<i> = NumberField(x^2+1) sage: gamma(i)
should not work at all. So either we should confirm it raises an error, check what "should" happen, or make it break...
comment:21 followup: ↓ 25 Changed 6 years ago by
 Reviewers changed from Burcin Erocal to Burcin Erocal, KarlDieter Crisman
Burcin, in case you're reading this  what was your intent with that doctest? Should this not work because an embedding needs to be specified, or should it "just work"?
comment:22 Changed 6 years ago by
comment:23 Changed 6 years ago by
 Commit changed from 1a00c65b8ee20bc16c9e36e0e1936f56fc8c86b9 to 489c5a1d1e1a3e700f62062a94f8190a4ade296e
Branch pushed to git repo; I updated commit sha1. New commits:
489c5a1  Merge branch 'develop' into t/16023/ticket/16023

comment:24 in reply to: ↑ 20 Changed 6 years ago by
 In other.py, in the
__call__
method for gamma, there is still an import of deprecation which is now not needed.
Still needed, I believe.
 This comment suggests that perhaps
sage: Q.<i> = NumberField(x^2+1) sage: gamma(i)should not work at all. So either we should confirm it raises an error, check what "should" happen, or make it break...
Given that e^i
, ln(i)
, and sqrt(i)
all fail in this case for various reasons related to embedding, maybe this should really be broken now. Since
if not str(err).startswith("cannot coerce"): raise
suggests that this is the only kind of error we treat, and then we just plop it in RR or CC as we can, I suppose we could remove that entire thing and do
return GinacFunction.__call__(self, x, coerce=coerce, hold=hold)
as the entire body of call or even remove it. What do you think?
comment:25 in reply to: ↑ 21 Changed 6 years ago by
Replying to kcrisman:
Burcin, in case you're reading this  what was your intent with that doctest? Should this not work because an embedding needs to be specified, or should it "just work"?
Right. It shouldn't work without specifying an explicit embedding in the NumberField
constructor.
I don't have time to try the patch now. It would be nice to give the user a helpful error message in this case, but I have no idea how much trouble that is.
comment:26 followup: ↓ 27 Changed 5 years ago by
Thanks, Burcin!
Okay, I will look at this today, then. Here are a few notes to myself so I don't forget (they are probably taken care of elsewhere or not needed):
 make sure 13608 is still doctested
 check whether 2607, 6094, 10859 were doctested and remove those if needed
 see if just raising a slightly different error message in the number field case will be enough
comment:27 in reply to: ↑ 26 Changed 5 years ago by
 Status changed from needs_review to needs_work
 make sure 13608 is still doctested
Yeah, although something similar is tested in symbolic/function.pyx this is testing something very specific to exp, so that test should be kept, probably just in the same place as the other example, or in some generic tests place in the file.
 check whether 2607, 6094, 10859 were doctested and remove those if needed
The 2607 stuff actually just removes the deprecated method for local max/min, but there is also the deprecated function it depended on which we might as well remove at the same time, seems weird not to
numerical/optimize.py:233:find_maximum_on_interval = deprecated_function_alias(2607, find_local_maximum) numerical/optimize.py:234:find_minimum_on_interval = deprecated_function_alias(2607, find_local_minimum)
These don't seem to have been tested, though, which is good for this ticket. The stuff for 6094 I found is unrelated to this ticket and not high priority, and 10859 was already all set.
 see if just raising a slightly different error message in the number field case will be enough
Here is current status if I just remove the call method entirely like you did the others.
sage: gamma(i).n() 0.154949828301811  0.498015668118356*I sage: Q.<i> = NumberField(x^2+1) sage: gamma(i)  TypeError: cannot coerce arguments: no canonical coercion from Number Field in i with defining polynomial x^2 + 1 to Symbolic Ring
I guess we could keep the method and raise an error about embedding, or keep this one. This is the only doctest failure in either directory, and we would want to rescue the tests from that module.
What do you think? I hope it's okay with the multi back and forth on this.
comment:28 followup: ↓ 29 Changed 5 years ago by
Well then, is this still really needs_work?
comment:29 in reply to: ↑ 28 Changed 5 years ago by
Well then, is this still really needs_work?
Yes, because currently
sage: Q.<i> = NumberField(x^2+1) sage: gamma(i)
doesn't just work, it is doctested! But the discussion above confirms it should not be allowed to work.
So, as I said above, we can either remove the call method entirely and then doctest that error, or keep it and make sure it raises an error about embedding as appropriate. We should also rescue the correct doctests from that method and put them somewhere as tests.
comment:30 Changed 5 years ago by
 Commit changed from 489c5a1d1e1a3e700f62062a94f8190a4ade296e to 633a4117fee037d40cb18adfc281d3451fad77dc
comment:31 Changed 5 years ago by
 Status changed from needs_work to needs_review
Ah ok. I should have looked into it better. I think after the buildbot thumbs up this can be set to positive?
comment:32 Changed 5 years ago by
Yup, thanks!
comment:33 Changed 5 years ago by
 Status changed from needs_review to positive_review
comment:34 Changed 5 years ago by
 Branch changed from u/rws/ticket/16023 to 633a4117fee037d40cb18adfc281d3451fad77dc
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac #16023: remove deprecated