Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Successive calls to Deriv influences result #2

Closed
notEvil opened this issue May 24, 2015 · 8 comments
Closed

Successive calls to Deriv influences result #2

notEvil opened this issue May 24, 2015 · 8 comments
Assignees
Labels

Comments

@notEvil
Copy link

notEvil commented May 24, 2015

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
Copy link
Author

notEvil commented May 25, 2015

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 sgsokol added the bug label May 26, 2015
@sgsokol sgsokol self-assigned this May 26, 2015
@sgsokol
Copy link
Owner

sgsokol commented May 26, 2015

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 sgsokol closed this as completed May 26, 2015
@sgsokol
Copy link
Owner

sgsokol commented May 26, 2015

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 #2 (comment).

@notEvil
Copy link
Author

notEvil commented May 26, 2015

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
#2 (comment).


Reply to this email directly or view it on GitHub
#2 (comment).

@sgsokol
Copy link
Owner

sgsokol commented May 27, 2015

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
Copy link
Author

notEvil commented May 27, 2015

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
#2 (comment).

@sgsokol
Copy link
Owner

sgsokol commented May 28, 2015

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
#2 (comment).


Reply to this email directly or view it on GitHub #2 (comment).

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
Copy link
Author

notEvil commented May 29, 2015

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
#2 (comment).


Reply to this email directly or view it on GitHub
#2 (comment).

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
#2 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants