sgsokol / Deriv

Symbolic differentiation
38 stars 4 forks source link

Successive calls to Deriv influences result #2

Closed notEvil closed 9 years ago

notEvil commented 9 years ago

Consider the following

Deriv::Deriv(quote(.e1*x), 'x')
Deriv::Deriv(quote(dnorm(x ** 2 - x)), 'x')
Deriv::Deriv(quote(.e1*x), 'x')
Deriv::Deriv(Deriv::Deriv(quote(dnorm(x ** 2 - x)), 'x'), 'x')
Deriv::Deriv(quote(.e1*x), 'x')

the last call to Deriv seems to be influenced by the previous one in that .e1 is somehow remembered. In my opinion this is a serious issue.

notEvil commented 9 years ago

hope you didn't already try to find the problem. It's the following :)

diff --git a/R/Deriv.R b/R/Deriv.R
index a9b0edc..2ee6dcf 100644
--- a/R/Deriv.R
+++ b/R/Deriv.R
@@ -164,7 +164,7 @@ Deriv <- function(f, x=if (is.function(f)) names(formals(f)) else all.vars(if (i
                f <- substitute(f)
        }
        # clean dsym env
-   rm(list=ls(dsym), envir=dsym)
+   rm(list=ls(dsym, all.names=T), envir=dsym)
        if (is.null(env))
                env <- .GlobalEnv
        if (is.null(x)) {
sgsokol commented 9 years ago

Thanks for the bug and fix. Unfortunately, the proposed solution fixes only the above example. There would still be issues with more complicated nested calls. It is fixed in the version 3.5

sgsokol commented 9 years ago

Hi,

I don't know if you have received automatic notifications from github, so may be this message is superfluous.

Both of your issues are fixed now (cf v3.5 on github).

If no new problem appears meanwhile, I'll submit this version to CRAN in two weeks.

Thanks for your interest and inputs.

Best, Serguei.

Le 25/05/2015 13:39, notEvil a écrit :

hope you didn't already try to find the problem. It's the following :)

|diff --git a/R/Deriv.R b/R/Deriv.R index a9b0edc..2ee6dcf 100644 --- a/R/Deriv.R +++ b/R/Deriv.R @@ -164,7 +164,7 @@ Deriv <- function(f, x=if (is.function(f)) names(formals(f)) else all.vars(if (i f <- substitute(f) }

clean dsym env

  • rm(list=ls(dsym), envir=dsym)
  • rm(list=ls(dsym, all.names=T), envir=dsym) if (is.null(env)) env <- .GlobalEnv if (is.null(x)) { |

— Reply to this email directly or view it on GitHub https://github.com/sgsokol/Deriv/issues/2#issuecomment-105215141.

notEvil commented 9 years ago

I have, thank you.

I can't promise to find any further bugs soon, so go ahead :) In the near future I plan to release an R package which benefits a lot from your package!

As for the issue: might Deriv still return a wrong but valid expression due to variable naming?

Best, Andreas

On 05/26/2015 05:38 PM, Serguei Sokol wrote:

Hi,

I don't know if you have received automatic notifications from github, so may be this message is superfluous.

Both of your issues are fixed now (cf v3.5 on github).

If no new problem appears meanwhile, I'll submit this version to CRAN in two weeks.

Thanks for your interest and inputs.

Best, Serguei.

Le 25/05/2015 13:39, notEvil a écrit :

hope you didn't already try to find the problem. It's the following :)

|diff --git a/R/Deriv.R b/R/Deriv.R index a9b0edc..2ee6dcf 100644 --- a/R/Deriv.R +++ b/R/Deriv.R @@ -164,7 +164,7 @@ Deriv <- function(f, x=if (is.function(f)) names(formals(f)) else all.vars(if (i f <- substitute(f) }

clean dsym env

  • rm(list=ls(dsym), envir=dsym)
  • rm(list=ls(dsym, all.names=T), envir=dsym) if (is.null(env)) env <- .GlobalEnv if (is.null(x)) { |

— Reply to this email directly or view it on GitHub https://github.com/sgsokol/Deriv/issues/2#issuecomment-105215141.

— Reply to this email directly or view it on GitHub https://github.com/sgsokol/Deriv/issues/2#issuecomment-105570522.

sgsokol commented 9 years ago

Le 26/05/2015 20:43, notEvil a écrit :

I have, thank you.

I can't promise to find any further bugs soon, so go ahead :) In the near future I plan to release an R package which benefits a lot from your package! To my knowledge, it will be premiere :)

As for the issue: might Deriv still return a wrong but valid expression due to variable naming? I am afraid, I don't understand the question. Do you have an example of such situation?

Btw, there is v3.5.1 now with a small fix in Cache() function.

Best, Sergueï.

notEvil commented 9 years ago

I was refering to your statement "Unfortunately, the proposed solution fixes only the above example. There would still be issues with more complicated nested calls."

I didn't know whether you meant that the fix is incomplete (as in there exists a general fix) or that it's just a fix for this particular sequence and it's still unsafe if there are nested calls, even with the new version.

Imagine the expression consists of .eXYZ and Deriv (or better Simplify) decides to create a cached expression with the same name, this would result in an expression which would still evaluate but is simply wrong.

Hope it clears things up.

Best, Andreas

On 05/27/2015 03:55 PM, Serguei Sokol wrote:

Le 26/05/2015 20:43, notEvil a écrit :

I have, thank you.

I can't promise to find any further bugs soon, so go ahead :) In the near future I plan to release an R package which benefits a lot from your package! To my knowledge, it will be premiere :)

As for the issue: might Deriv still return a wrong but valid expression due to variable naming? I am afraid, I don't understand the question. Do you have an example of such situation?

Btw, there is v3.5.1 now with a small fix in Cache() function.

Best, Sergueï.

— Reply to this email directly or view it on GitHub https://github.com/sgsokol/Deriv/issues/2#issuecomment-105920542.

sgsokol commented 9 years ago

Le 27/05/2015 17:51, notEvil a écrit :

I was refering to your statement "Unfortunately, the proposed solution fixes only the above example. There would still be issues with more complicated nested calls."

I didn't know whether you meant that the fix is incomplete (as in there exists a general fix) or that it's just a fix for this particular sequence and it's still unsafe if there are nested calls, even with the new version.

Imagine the expression consists of .eXYZ and Deriv (or better Simplify) decides to create a cached expression with the same name, this would result in an expression which would still evaluate but is simply wrong.

Hope it clears things up. Yes, it is clear now. Your example of nested call is particular in a sens that the innermost call to Deriv() is made before than the outermost one has something to do. Imagine a different call structure like this: Deriv some operations modifying dsym Deriv (here the fact that dsym is erased can interfer this the precedent call to Deriv) some other operations needing original dsym

That's why, a general case of nested calls to Deriv requires a local dsym (and scache by the same occasion).

Hope it helps. Serguei.

Best, Andreas

On 05/27/2015 03:55 PM, Serguei Sokol wrote:

Le 26/05/2015 20:43, notEvil a écrit :

I have, thank you.

I can't promise to find any further bugs soon, so go ahead :) In the near future I plan to release an R package which benefits a lot from your package! To my knowledge, it will be premiere :)

As for the issue: might Deriv still return a wrong but valid expression due to variable naming? I am afraid, I don't understand the question. Do you have an example of such situation?

Btw, there is v3.5.1 now with a small fix in Cache() function.

Best, Sergueï.

— Reply to this email directly or view it on GitHub https://github.com/sgsokol/Deriv/issues/2#issuecomment-105920542.

— Reply to this email directly or view it on GitHub https://github.com/sgsokol/Deriv/issues/2#issuecomment-105970694.

Serguei Sokol Ingenieur de recherche INRA Metabolisme Integre et Dynamique des Systemes Metaboliques (MetaSys)

LISBP, INSA/INRA UMR 792, INSA/CNRS UMR 5504 135 Avenue de Rangueil 31077 Toulouse Cedex 04

tel: +33 5 6155 9276 fax: +33 5 6704 8825 email: sokol@insa-toulouse.fr http://metasys.insa-toulouse.fr http://www.lisbp.fr

notEvil commented 9 years ago

Now everything is clear to me. Thank you for your time to explain.

On 05/28/2015 01:28 PM, Serguei Sokol wrote:

Le 27/05/2015 17:51, notEvil a écrit :

I was refering to your statement "Unfortunately, the proposed solution fixes only the above example. There would still be issues with more complicated nested calls."

I didn't know whether you meant that the fix is incomplete (as in there exists a general fix) or that it's just a fix for this particular sequence and it's still unsafe if there are nested calls, even with the new version.

Imagine the expression consists of .eXYZ and Deriv (or better Simplify) decides to create a cached expression with the same name, this would result in an expression which would still evaluate but is simply wrong.

Hope it clears things up. Yes, it is clear now. Your example of nested call is particular in a sens that the innermost call to Deriv() is made before than the outermost one has something to do. Imagine a different call structure like this: Deriv some operations modifying dsym Deriv (here the fact that dsym is erased can interfer this the precedent call to Deriv) some other operations needing original dsym

That's why, a general case of nested calls to Deriv requires a local dsym (and scache by the same occasion).

Hope it helps. Serguei.

Best, Andreas

On 05/27/2015 03:55 PM, Serguei Sokol wrote:

Le 26/05/2015 20:43, notEvil a écrit :

I have, thank you.

I can't promise to find any further bugs soon, so go ahead :) In the near future I plan to release an R package which benefits a lot from your package! To my knowledge, it will be premiere :)

As for the issue: might Deriv still return a wrong but valid expression due to variable naming? I am afraid, I don't understand the question. Do you have an example of such situation?

Btw, there is v3.5.1 now with a small fix in Cache() function.

Best, Sergueï.

— Reply to this email directly or view it on GitHub https://github.com/sgsokol/Deriv/issues/2#issuecomment-105920542.

— Reply to this email directly or view it on GitHub https://github.com/sgsokol/Deriv/issues/2#issuecomment-105970694.

Serguei Sokol Ingenieur de recherche INRA Metabolisme Integre et Dynamique des Systemes Metaboliques (MetaSys)

LISBP, INSA/INRA UMR 792, INSA/CNRS UMR 5504 135 Avenue de Rangueil 31077 Toulouse Cedex 04

tel: +33 5 6155 9276 fax: +33 5 6704 8825 email: sokol@insa-toulouse.fr http://metasys.insa-toulouse.fr http://www.lisbp.fr

— Reply to this email directly or view it on GitHub https://github.com/sgsokol/Deriv/issues/2#issuecomment-106284624.