remove deprecated code in functions/ and symbolic/ #16023

rwst · 2014-03-27T15:18:46Z

Component: symbolics

Keywords: deprecation

Author: Ralf Stephan

Branch/Commit: 633a411

Reviewer: Burcin Erocal, Karl-Dieter Crisman

Issue created by migration from https://trac.sagemath.org/ticket/16023

The text was updated successfully, but these errors were encountered:

rwst · 2014-03-27T15:21:45Z

New commits:

`0b379e8`	`Trac #16023: remove deprecated`

rwst · 2014-03-27T15:21:45Z

Commit: 0b379e8

rwst · 2014-03-27T15:21:45Z

Branch: u/rws/ticket/16023

burcin · 2014-03-27T16:52:00Z

Reviewer: Burcin Erocal

burcin · 2014-03-27T16:52:00Z

comment:2

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.

sagetrac-git · 2014-03-27T17:27:36Z

Changed commit from 0b379e8 to eb0fcad

sagetrac-git · 2014-03-27T17:27:36Z

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

`eb0fcad`	remove superfluous `__call__` funcs

rwst · 2014-03-27T17:29:03Z

comment:4

Thanks. I believe you mean Ei and exp.

burcin · 2014-03-27T17:33:19Z

comment:5

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?)

sagetrac-git · 2014-03-27T17:35:09Z

Changed commit from eb0fcad to 4a9b2e1

sagetrac-git · 2014-03-27T17:35:09Z

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

`4a9b2e1`	`fix typo`

rwst · 2014-03-29T08:31:48Z

comment:7

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:
sagemath/sage-patchbot#10

Thanks.

kcrisman · 2014-04-01T14:33:05Z

comment:8

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 https://github.com/sagemath/sagetrac-mirror/blob/develop/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.

rwst · 2014-04-15T07:08:49Z

Changed commit from 4a9b2e1 to e964cf5

rwst · 2014-04-15T07:08:49Z

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`

rwst · 2014-04-15T07:08:49Z

Changed branch from u/rws/ticket/16023 to u/rws/ticket/16023-1

rwst · 2014-04-15T07:49:19Z

Changed commit from e964cf5 to 4a9b2e1

rwst · 2014-04-15T07:49:19Z

comment:10

Oops wrong branch.

rwst · 2014-04-15T07:49:19Z

Changed branch from u/rws/ticket/16023-1 to u/rws/ticket/16023

kcrisman · 2014-04-15T19:04:44Z

comment:11

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

sagetrac-git · 2014-04-16T14:14:02Z

Changed commit from 4a9b2e1 to 2b23263

sagetrac-git · 2014-04-16T14:14:02Z

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

`01dc349`	`Merge branch 'u/rws/ticket/16023' of trac.sagemath.org:sage into tmp`
`2b23263`	`16023: remove deprecated keyword code`

sagetrac-git · 2014-07-28T13:15:58Z

Changed commit from b856623 to 1a00c65

kcrisman · 2014-08-15T04:28:48Z

comment:20

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...

kcrisman · 2014-08-15T04:31:09Z

comment:21

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"?

kcrisman · 2014-08-15T04:31:09Z

Changed reviewer from Burcin Erocal to Burcin Erocal, Karl-Dieter Crisman

kcrisman · 2014-08-15T07:37:11Z

comment:22

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

sagetrac-git · 2014-08-20T14:03:43Z

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

`489c5a1`	`Merge branch 'develop' into t/16023/ticket/16023`

sagetrac-git · 2014-08-20T14:03:43Z

Changed commit from 1a00c65 to 489c5a1

kcrisman · 2014-08-27T19:18:43Z

comment:24

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?

burcin · 2014-08-28T03:19:54Z

comment:25

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.

kcrisman · 2014-08-28T14:22:14Z

comment:26

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

kcrisman · 2014-08-28T15:46:01Z

comment:27

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.

rwst · 2014-09-03T09:33:40Z

comment:28

Well then, is this still really needs_work?

kcrisman · 2014-09-03T12:28:46Z

comment:29

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.

sagetrac-git · 2014-09-03T14:11:23Z

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

`186e330`	`Merge branch 'develop' into t/16023/ticket/16023`
`633a411`	`16023: remove gamma1.__call__()`

sagetrac-git · 2014-09-03T14:11:23Z

Changed commit from 489c5a1 to 633a411

rwst · 2014-09-03T14:13:45Z

comment:31

Ah ok. I should have looked into it better. I think after the buildbot thumbs up this can be set to positive?

kcrisman · 2014-09-03T15:21:50Z

comment:32

Yup, thanks!

vbraun · 2014-09-06T11:02:57Z

Changed branch from u/rws/ticket/16023 to 633a411

rwst added this to the sage-6.2 milestone Mar 27, 2014

rwst added c: symbolics labels Mar 27, 2014

rwst added the s: needs review label Mar 27, 2014

burcin added s: needs work and removed s: needs review labels Mar 27, 2014

rwst added s: needs review and removed s: needs work labels Mar 27, 2014

sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014

kcrisman added s: needs work and removed s: needs review labels Aug 28, 2014

rwst added s: needs review and removed s: needs work labels Sep 3, 2014

rwst added s: positive review and removed s: needs review labels Sep 4, 2014

vbraun removed the s: positive review label Sep 6, 2014

vbraun closed this as completed in 126311d Sep 6, 2014

This was referenced Aug 16, 2014

remove _Bessel functions #16173

Closed

clean and speed up doctesting of bessel.py #16737

Closed

remove deprecated code in functions/ and symbolic/ #16023

remove deprecated code in functions/ and symbolic/ #16023

Comments

rwst commented Mar 27, 2014

rwst commented Mar 27, 2014

rwst commented Mar 27, 2014

rwst commented Mar 27, 2014

burcin commented Mar 27, 2014

burcin commented Mar 27, 2014

sagetrac-git mannequin commented Mar 27, 2014

sagetrac-git mannequin commented Mar 27, 2014

rwst commented Mar 27, 2014

burcin commented Mar 27, 2014

sagetrac-git mannequin commented Mar 27, 2014

sagetrac-git mannequin commented Mar 27, 2014

rwst commented Mar 29, 2014

kcrisman commented Apr 1, 2014

rwst commented Apr 15, 2014

rwst commented Apr 15, 2014

rwst commented Apr 15, 2014

rwst commented Apr 15, 2014

rwst commented Apr 15, 2014

rwst commented Apr 15, 2014

kcrisman commented Apr 15, 2014

sagetrac-git mannequin commented Apr 16, 2014

sagetrac-git mannequin commented Apr 16, 2014

sagetrac-git mannequin commented Jul 28, 2014

kcrisman commented Aug 15, 2014

kcrisman commented Aug 15, 2014

kcrisman commented Aug 15, 2014

kcrisman commented Aug 15, 2014

sagetrac-git mannequin commented Aug 20, 2014

sagetrac-git mannequin commented Aug 20, 2014

kcrisman commented Aug 27, 2014

burcin commented Aug 28, 2014

kcrisman commented Aug 28, 2014

kcrisman commented Aug 28, 2014

rwst commented Sep 3, 2014

kcrisman commented Sep 3, 2014

sagetrac-git mannequin commented Sep 3, 2014

sagetrac-git mannequin commented Sep 3, 2014

rwst commented Sep 3, 2014

kcrisman commented Sep 3, 2014

vbraun commented Sep 6, 2014