search

BennMacdonald / AGM_RPackage

1 stars 0 forks source link

Default settings for defaultTemperingScheme and defaultPrior #15

Closed FrankD closed 7 years ago

FrankD commented 7 years ago

If defaultTemperingScheme and defaultPrior are not set when calling the agm, the code throws an error, even if tempering is not actually used (temperMismatchParameter=FALSE).

I've observed two error messages associated with this:

Error in if (auxVars$defaultLogParamPrior == "Uniform") { : 
  argument is of length zero

and

Error in lambda[chain, species] : incorrect number of dimensions

Easy fix would be to set these parameters to some default in the agm function call, but I'd like to understand why this causes an issue if tempering is not used? Any ideas Benn?

BennMacdonald commented 7 years ago

Could you give me a copy of what you ran as the arguments of the function i.e. agm(...), so that I can reproduce? Cheers.

FrankD commented 7 years ago 
agm(test.data,test.times,LV_func,4,noise.sd=0.1,temperMismatchParameter=FALSE)

with test.data and test.times as in the vignette.

BennMacdonald commented 7 years ago

Cheers. I will look at this today and get back to you.

BennMacdonald commented 7 years ago

Quick question r.e. priors: Did we implement a way for users to pass through any prior they want? I thought we had, but trying to locate it.

If we didn't, I think I know the problem for one of the warnings.

FrankD commented 7 years ago

I don't think we have, at least I don't remember doing it...

On 26/07/2017 14:18, BennMacdonald wrote:

Quick question r.e. priors: Did we implement a way for users to pass through any prior they want? I thought we had, but trying to locate it.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub https://github.com/BennMacdonald/AGM_RPackage/issues/15#issuecomment-318050396, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAstkM2ZwYelJpOaV6X7yzeHVcBkyfrks5sRzzDgaJpZM4Od9QI.

BennMacdonald commented 7 years ago

Ah, I think that is the problem then. Currently, we have defaultPrior=NULL as the default in the function agm(...). Later we write

if(!is.null(defaultPrior)){ auxVars$defaultLogParamPrior <- defaultPrior } in order to pass it to the auxVars variable. The plan was for this to be passed if and only if a default prior was used. We should have put in a way for users to specify there own prior and then update the function calculateLogParamPrior to only use the defaultLogParamPrior if it is not empty.

Currently, because the settings agm(test.data,test.times,LV_func,4,noise.sd=0.1,temperMismatchParameter=FALSE) does not specify a default prior, auxVars$defaultLogParamPrior does not exist and the statement if(auxVars$defaultLogParamPrior=="Uniform") cannot be evaluated, returning the warning you saw.

I will update the code to avoid this. I will also look at allowing users to specify their own prior.

Can you run this: agm(test.data,test.times,LV_func,4,noise.sd=0.1,temperMismatchParameter=FALSE, defaultPrior="Uniform",showProgress=TRUE,showPlot=TRUE) and see if you encounter the error - I did not. I am not sure how to produce the second error you got. Can you see if you can reproduce it and let me know?

BennMacdonald commented 7 years ago

Hi Frank. I updated the code to allow for user specified priors. If the user wishes to specify their own prior, they simply create a function called userLogPrior in the workspace and make sure defaultPrior is not invoked i.e. set to NULL. The form of userLogPrior is pretty simple and I will show later on. In agm() I changed

if(!is.null(defaultPrior)){ auxVars$defaultLogParamPrior <- defaultPrior }

to now be able to catch no specified prior

if(!is.null(defaultPrior)){ auxVars$defaultLogParamPrior <- defaultPrior }else{ auxVars$defaultLogParamPrior <- NULL if(!(exists("userLogPrior")&&is.function(userLogPrior))) { stop(warning="You must create userLogPrior in workspace or use defaultPrior option in agm()") } }

and I edited calculateLogParamPrior to identify when default prior is specified (in which case it will be used) and when it is NULL (in which case the user must specify their own log prior - userLogPrior).

The following code can be used as a test case:

agm(test.data,test.times,LV_func,4,noise.sd=0.1,temperMismatchParameter=FALSE, defaultPrior="Uniform",showProgress=TRUE,showPlot=TRUE)

Works with defaultPrior - this should run

agm(test.data,test.times,LV_func,4,noise.sd=0.1,temperMismatchParameter=FALSE, showProgress=TRUE,showPlot=TRUE)

Should not work as no prior has been specified - this should not run

Put below in workspace

userLogPrior <- function(params) { c(dgamma(params[1:3],1,1,log=TRUE),dgamma(params[4],1,2,log=TRUE)) }

General form to be explained in vignette

agm(test.data,test.times,LV_func,4,noise.sd=0.1,temperMismatchParameter=FALSE, showProgress=TRUE,showPlot=TRUE)

Now works with user specified prior. If defaultPrior is invoked, the user specified prior will be ignored

Let me know what you think.

FrankD commented 7 years ago

Good work, thanks Benn! I can't look at it in detail at the moment, but what you wrote makes sense to me. Will do the tests once I'm back at work and get back to you.

FrankD commented 7 years ago

This works fine for me in testing, but I would change the way the user-defined prior is invoked. Rather than putting a function with a specific name in the workspace (error-prone), why not pass the in userLogPrior as an argument to agm. In fact, we can make it even simpler and avoid the error message by saying that if userLogPrior is not provided as an argument, then we use the default ('Uniform').

If you're fine with that, then I will make the changes and close this issue.

BennMacdonald commented 7 years ago

User specific prior is now implemented, along the lines you suggest above.

Can you let me know if you are able to produce the second error in the original post of this thread? If you cannot, we can close the issue.

FrankD commented 7 years ago

Thanks Benn! I have made a slight modification (combined the two parameters defaultLogPrior and userLogPrior into one parameter logPrior that can be either a string or a user-defined function). This seems to work fine now.

Small request, before you commit changes, can you run the devtools::test() function to check that it passes all the unit tests?

The other error has not re-occurred, so I think it must also have been related to the issue with the prior.

BennMacdonald commented 7 years ago

Hi Frank,

Question regarding the unit tests (I am still new to this): I run devtools::test(“deGradInfer”) and receive the following output

Loading deGradInfer Loading required package: testthat Testing deGradInfer Arguments Tests, 50 Iterations: .. Likelihood Comparison: 1234 Likelihood Improvement, 500 Iterations: .

Failed -------------------------------------------------------------------------

  1. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_1.rds. Component “K.u”: Mean relative difference: 7.326002e-08

  2. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_2.rds. Component “K.u”: Mean relative difference: 7.326002e-08

  3. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_3.rds. Component “K.u”: Mean relative difference: 7.326002e-08

  4. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_4.rds. Component “K.u”: Mean relative difference: 1.622975e-08

DONE =======================

Would this be acceptable to push to GitHub as the relative difference is so small?

Regards, Dr Benn Macdonald

Biometrika Fellow University of Glasgow School of Mathematics and Statistics Room 225 G12 8SQ

From: FrankDmailto:notifications@github.com Sent: 24 August 2017 16:38 To: BennMacdonald/AGM_RPackagemailto:AGM_RPackage@noreply.github.com Cc: Benn Macdonaldmailto:Benn.MacDonald@glasgow.ac.uk; Assignmailto:assign@noreply.github.com Subject: Re: [BennMacdonald/AGM_RPackage] Default settings for defaultTemperingScheme and defaultPrior (#15)

Thanks Benn! I have made a slight modification (combined the two parameters defaultLogPrior and userLogPrior into one parameter logPrior that can be either a string or a user-defined function). This seems to work fine now.

Small request, before you commit changes, can you run the devtools::test() function to check that it passes all the unit tests?

The other error has not re-occurred, so I think it must also have been related to the issue with the prior.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHubhttps://github.com/BennMacdonald/AGM_RPackage/issues/15#issuecomment-324673321, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AZQZ2dNJE3bJgG_kTq6k_c-P_3z-i8aQks5sbZkGgaJpZM4Od9QI.

FrankD commented 7 years ago

Yes, that seems fine, it's likely a difference in the machine precision between Windows and Linux versions of R (especially since it's the same difference for all three quantities).

On 07/09/2017 16:28, BennMacdonald wrote:

Hi Frank,

Question regarding the unit tests (I am still new to this): I run devtools::test(“deGradInfer”) and receive the following output

Loading deGradInfer Loading required package: testthat Testing deGradInfer Arguments Tests, 50 Iterations: .. Likelihood Comparison: 1234 Likelihood Improvement, 500 Iterations: .

Failed

  1. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_1.rds. Component “K.u”: Mean relative difference: 7.326002e-08

  2. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_2.rds. Component “K.u”: Mean relative difference: 7.326002e-08

  3. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_3.rds. Component “K.u”: Mean relative difference: 7.326002e-08

  4. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_4.rds. Component “K.u”: Mean relative difference: 1.622975e-08

DONE =======================

Would this be acceptable to push to GitHub as the relative difference is so small?

Regards, Dr Benn Macdonald

Biometrika Fellow University of Glasgow School of Mathematics and Statistics Room 225 G12 8SQ

From: FrankDmailto:notifications@github.com Sent: 24 August 2017 16:38 To: BennMacdonald/AGM_RPackagemailto:AGM_RPackage@noreply.github.com Cc: Benn Macdonaldmailto:Benn.MacDonald@glasgow.ac.uk; Assignmailto:assign@noreply.github.com Subject: Re: [BennMacdonald/AGM_RPackage] Default settings for defaultTemperingScheme and defaultPrior (#15)

Thanks Benn! I have made a slight modification (combined the two parameters defaultLogPrior and userLogPrior into one parameter logPrior that can be either a string or a user-defined function). This seems to work fine now.

Small request, before you commit changes, can you run the devtools::test() function to check that it passes all the unit tests?

The other error has not re-occurred, so I think it must also have been related to the issue with the prior.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHubhttps://github.com/BennMacdonald/AGM_RPackage/issues/15#issuecomment-324673321, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AZQZ2dNJE3bJgG_kTq6k_c-P_3z-i8aQks5sbZkGgaJpZM4Od9QI.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/BennMacdonald/AGM_RPackage/issues/15#issuecomment-327835245, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAstkQSx7RWsB95M602WxuqjQK4Cf5Tks5sgAuegaJpZM4Od9QI.

BennMacdonald commented 7 years ago

Perfect. Also, thanks for getting back to me so quickly even although you are out of office. I will push the changes to GitHub. Out of the things you listed left to be done

Regards, Dr Benn Macdonald

Biometrika Fellow University of Glasgow School of Mathematics and Statistics Room 225 G12 8SQ

From: FrankDmailto:notifications@github.com Sent: 07 September 2017 16:34 To: BennMacdonald/AGM_RPackagemailto:AGM_RPackage@noreply.github.com Cc: Benn Macdonaldmailto:Benn.MacDonald@glasgow.ac.uk; Assignmailto:assign@noreply.github.com Subject: Re: [BennMacdonald/AGM_RPackage] Default settings for defaultTemperingScheme and defaultPrior (#15)

Yes, that seems fine, it's likely a difference in the machine precision between Windows and Linux versions of R (especially since it's the same difference for all three quantities).

On 07/09/2017 16:28, BennMacdonald wrote:

Hi Frank,

Question regarding the unit tests (I am still new to this): I run devtools::test(“deGradInfer”) and receive the following output

Loading deGradInfer Loading required package: testthat Testing deGradInfer Arguments Tests, 50 Iterations: .. Likelihood Comparison: 1234 Likelihood Improvement, 500 Iterations: .

Failed

  1. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_1.rds. Component “K.u”: Mean relative difference: 7.326002e-08

  2. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_2.rds. Component “K.u”: Mean relative difference: 7.326002e-08

  3. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_3.rds. Component “K.u”: Mean relative difference: 7.326002e-08

  4. Failure: Likelihood is unchanged (@test_likelihood_calculation.R#19) -------- LL not equal to reference from ll_4.rds. Component “K.u”: Mean relative difference: 1.622975e-08

DONE =======================

Would this be acceptable to push to GitHub as the relative difference is so small?

Regards, Dr Benn Macdonald

Biometrika Fellow University of Glasgow School of Mathematics and Statistics Room 225 G12 8SQ

From: FrankDmailto:notifications@github.com Sent: 24 August 2017 16:38 To: BennMacdonald/AGM_RPackagemailto:AGM_RPackage@noreply.github.com Cc: Benn Macdonaldmailto:Benn.MacDonald@glasgow.ac.uk; Assignmailto:assign@noreply.github.com Subject: Re: [BennMacdonald/AGM_RPackage] Default settings for defaultTemperingScheme and defaultPrior (#15)

Thanks Benn! I have made a slight modification (combined the two parameters defaultLogPrior and userLogPrior into one parameter logPrior that can be either a string or a user-defined function). This seems to work fine now.

Small request, before you commit changes, can you run the devtools::test() function to check that it passes all the unit tests?

The other error has not re-occurred, so I think it must also have been related to the issue with the prior.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHubhttps://github.com/BennMacdonald/AGM_RPackage/issues/15#issuecomment-324673321, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AZQZ2dNJE3bJgG_kTq6k_c-P_3z-i8aQks5sbZkGgaJpZM4Od9QI.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/BennMacdonald/AGM_RPackage/issues/15#issuecomment-327835245, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAstkQSx7RWsB95M602WxuqjQK4Cf5Tks5sgAuegaJpZM4Od9QI.

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHubhttps://github.com/BennMacdonald/AGM_RPackage/issues/15#issuecomment-327837023, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AZQZ2dYfeU05pjpnYKgROeGEYGAp3kwAks5sgAz8gaJpZM4Od9QI.