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: sage-6.4
Component: symbolics Keywords: deprecation
Cc: Merged in:
Authors: Ralf Stephan Reviewers: Burcin Erocal, Karl-Dieter 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 rws

  • Branch set to u/rws/ticket/16023
  • Commit set to 0b379e829059b3c95faa21176e46c75473baff96
  • Status changed from new to needs_review

New commits:

0b379e8Trac #16023: remove deprecated

comment:2 Changed 6 years ago by burcin

  • 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 git

  • Commit changed from 0b379e829059b3c95faa21176e46c75473baff96 to eb0fcadcb6adeab9d97d58ab1db45ad340c1b76e

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

eb0fcadremove superfluous __call__ funcs

comment:4 Changed 6 years ago by rws

  • Status changed from needs_work to needs_review

Thanks. I believe you mean Ei and exp.

comment:5 follow-up: Changed 6 years ago by 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?)

comment:6 Changed 6 years ago by git

  • Commit changed from eb0fcadcb6adeab9d97d58ab1db45ad340c1b76e to 4a9b2e19851e68bae5106f1ae8e27876b107de69

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

4a9b2e1fix typo

comment:7 in reply to: ↑ 5 Changed 6 years ago by rws

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/sage-patchbot/issues/10

Thanks.

comment:8 follow-up: Changed 6 years ago by 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.

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 rws

  • Branch changed from u/rws/ticket/16023 to u/rws/ticket/16023-1
  • Commit changed from 4a9b2e19851e68bae5106f1ae8e27876b107de69 to e964cf5f5a6553ba90aac03dd1e7822d4e5dcece

Last 10 new commits:

ba8a5a6ref doc completed; coefficients(), slicing added
be9bb7fhandle ogf > 1
b39485bmove to rings/, allow 1/x, subclass from FractionFieldElement
cc2b400add, sub functions
998ed74mul, div
ca22f6dbe specific about base ring, more doctests
8b74445specify applicability, handle constants, more doctests, better documentation
a9a19a9use pari script default for guessing; safeguards in ctor
662a41blast doctests, fix documentation
e964cf5Merge branch 'u/rws/ticket/16023' of trac.sagemath.org:sage into tmp

comment:10 Changed 6 years ago by rws

  • Branch changed from u/rws/ticket/16023-1 to u/rws/ticket/16023
  • Commit changed from e964cf5f5a6553ba90aac03dd1e7822d4e5dcece to 4a9b2e19851e68bae5106f1ae8e27876b107de69

Oops wrong branch.

comment:11 Changed 6 years ago by kcrisman

Now the branch doesn't seem to be coming up...

comment:12 Changed 6 years ago by git

  • Commit changed from 4a9b2e19851e68bae5106f1ae8e27876b107de69 to 2b232633fbdc27c73f3eb77163217bbeda773338

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

01dc349Merge branch 'u/rws/ticket/16023' of trac.sagemath.org:sage into tmp
2b2326316023: remove deprecated keyword code

comment:13 in reply to: ↑ 8 ; follow-up: Changed 6 years ago by rws

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 kcrisman

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 git

  • Commit changed from 2b232633fbdc27c73f3eb77163217bbeda773338 to 1946f517f92292231eb244ad2033dbe16e755244

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

1946f51Merge branch 'develop' into t/16023/ticket/16023

comment:16 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:17 Changed 6 years ago by git

  • Commit changed from 1946f517f92292231eb244ad2033dbe16e755244 to b856623e25d8261d7442433a0b1fafd13b8cc1bf

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

189bb39Merge branch 'develop' into t/16023/ticket/16023
b85662316023: add doctest

comment:18 Changed 6 years ago by git

  • Commit changed from b856623e25d8261d7442433a0b1fafd13b8cc1bf to 1a00c65b8ee20bc16c9e36e0e1936f56fc8c86b9

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

1a00c65Merge branch 'develop' into t/16023/ticket/16023

comment:19 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:20 follow-up: Changed 6 years ago by kcrisman

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 follow-up: Changed 6 years ago by kcrisman

  • Reviewers changed from Burcin Erocal to Burcin Erocal, Karl-Dieter 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 kcrisman

Also, after #16173 and #16737 this one will probably be a little smaller.

comment:23 Changed 6 years ago by git

  • Commit changed from 1a00c65b8ee20bc16c9e36e0e1936f56fc8c86b9 to 489c5a1d1e1a3e700f62062a94f8190a4ade296e

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

489c5a1Merge branch 'develop' into t/16023/ticket/16023

comment:24 in reply to: ↑ 20 Changed 6 years ago by kcrisman

  • 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 burcin

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

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 kcrisman

  • 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 follow-up: Changed 5 years ago by rws

Well then, is this still really needs_work?

comment:29 in reply to: ↑ 28 Changed 5 years ago by kcrisman

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 git

  • Commit changed from 489c5a1d1e1a3e700f62062a94f8190a4ade296e to 633a4117fee037d40cb18adfc281d3451fad77dc

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

186e330Merge branch 'develop' into t/16023/ticket/16023
633a41116023: remove gamma1.__call__()

comment:31 Changed 5 years ago by rws

  • 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 kcrisman

Yup, thanks!

comment:33 Changed 5 years ago by rws

  • Status changed from needs_review to positive_review

comment:34 Changed 5 years ago by vbraun

  • Branch changed from u/rws/ticket/16023 to 633a4117fee037d40cb18adfc281d3451fad77dc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.