I1170 combined gaussian loglikelihood

[DavAug]

See PR #1182

This is probably not the right way to do it, but...

I checked out a previous version of the branch in #1182 and added some changes.

[ben18785]

Yep!

…

On Tue, Aug 18, 2020 at 3:49 PM David Augustin ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pints/_log_likelihoods.py <#1190 (comment)>: > @@ -237,6 +237,226 @@ def __call__(self, x): ) +class LinearHeteroscedasticGaussianLogLikelihood( That is correct! Hmm I don't know why I thought it's linear. It somehow sounded need :D. Anyway it's not correct. Well nonlinear is probably to general. Let's just go back to ConstantAndMultiplicativeGaussianError then? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1190 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCILKAZSVLUC7WKWTPDCUDSBKIHLANCNFSM4P24ZI3A> .

[DavAug]

@MichaelClerx @ben18785
I've incorporated the suggested changes. I also added a very brief discussion in the notebook how one may deduce that the MCMC run is not successful at first.

Let me know what you think. :)

[ben18785]

Looks good @DavAug -- thanks for working through these changes. A couple more things then good to go:

• Can you change the notebook title to "Inference using a two compartment error model incorporating constant and multiplicative errors". Only the first letter "I" should be capitalised.
• There are quite a few comments that start with #s in the cells themselves. Some of these are repetitive: for example, "Infer model parameters with Haario ACMCMC." repeats the text above it. Also, many of these comments at the top of code blocks should be moved outside of them: that'll make them look neater. Can you clean these up?

[DavAug]

Looks good @DavAug -- thanks for working through these changes. A couple more things then good to go:

• Can you change the notebook title to "Inference using a two compartment error model incorporating constant and multiplicative errors". Only the first letter "I" should be capitalised.
• There are quite a few comments that start with #s in the cells themselves. Some of these are repetitive: for example, "Infer model parameters with Haario ACMCMC." repeats the text above it. Also, many of these comments at the top of code blocks should be moved outside of them: that'll make them look neater. Can you clean these up?

Sure! Is this how those models are called "two compartment error model"?

[ben18785]

@DavAug no (I don't know how those models are called) but perhaps that's a better title?

[DavAug]

@ben18785
Thanks again for the review. I tried to incorporate your change requests. Would you prefer if I removed all comments on the top of the cells?

[ben18785]

No need to remove them -- just make the text markdown (i.e. move it above them) as, when they're in the cells, the comments look a bit messy.

[DavAug]

No need to remove them -- just make the text markdown (i.e. move it above them) as, when they're in the cells, the comments look a bit messy.

Sorry I think I'm a little slow right now.. Would you prefer to see

• the requirements for each cell in markdown above the cell,
• generally when there is a stream of multiple cells a markdown above each cell that contains the short comment that is currently at the top of each cell, or
• both of the above?

[ben18785]

Ha, sorry, this is quite hard to communicate and I'm not doing a good job of this. It's easier to explain with a few examples. In general, my point is, you aren't currently making sufficient use of markdown text. The comments written at the top of most cells (but within them) would look much nicer if they were in markdown.

Take the cell immediately below, "Show quantitative and visual diagnostics of MCMC runs". Within the cell, you have a comment, ``This cell needs the HaarioACMC chains'', which is written using #s as it's a comment in Python. This would look better if this comment were moved above the cell (and perhaps explained a little) and written in markdown (immediately below "Show quantitative and visual diagnostics of MCMC runs").

Similarly, take the cell with "Construct posterior" in it. This cell has no markdown comments above it currently. The stuff that's written from "Construct posterior. This cell needs the above defined true parameters, the generated data, and the model wrapper to be able to fix eta" could be moved up to markdown text. It could also perhaps be explained a bit better.

A separate thing that I've just noticed is that you seem to be reloading packages: for example, you import pints a number of times. Can we just have these once at the top (or inline if that makes it easier to understand)?

[DavAug]

Ha, sorry, this is quite hard to communicate and I'm not doing a good job of this. It's easier to explain with a few examples. In general, my point is, you aren't currently making sufficient use of markdown text. The comments written at the top of most cells (but within them) would look much nicer if they were in markdown.

Take the cell immediately below, "Show quantitative and visual diagnostics of MCMC runs". Within the cell, you have a comment, ``This cell needs the HaarioACMC chains'', which is written using #s as it's a comment in Python. This would look better if this comment were moved above the cell (and perhaps explained a little) and written in markdown (immediately below "Show quantitative and visual diagnostics of MCMC runs").

Similarly, take the cell with "Construct posterior" in it. This cell has no markdown comments above it currently. The stuff that's written from "Construct posterior. This cell needs the above defined true parameters, the generated data, and the model wrapper to be able to fix eta" could be moved up to markdown text. It could also perhaps be explained a bit better.

A separate thing that I've just noticed is that you seem to be reloading packages: for example, you import pints a number of times. Can we just have these once at the top (or inline if that makes it easier to understand)?

Ah ok! I'm happy to do those changes :)

The reason why I include the requirements, i.e. "this cell needs...", and the imports in each cell is that I would like to avoid the problem of notebooks that the order of execution matters. Most notebooks only work from top to bottom. This way I highlight which cells need to be executed first. By including the relevant imports in each cell, I avoid that kind of dependence between cells. So the in cell comments display the variable dependence of that cell on other cells.

I hope that makes sense. But if you have strong feelings about this, I am happy to change it.

[ben18785]

Makes sense but I think we, in general, try to have the notebooks so that they run from top to bottom and only import packages once (I'm not sure we always manage to do this though!). So can we change to make it like this?

[DavAug]

Makes sense but I think we, in general, try to have the notebooks so that they run from top to bottom and only import packages once (I'm not sure we always manage to do this though!). So can we change to make it like this?

Yep :)

[ben18785]

Looks great! Thanks @DavAug

[ben18785]

@MichaelClerx Not sure if there were any changes outstanding with this? I'm happy for this to be merged.

[MichaelClerx]

I've merged it in. Thanks all!

[ben18785]

Great, thanks Michael!

…

On Thu, Aug 27, 2020 at 1:05 PM Michael Clerx ***@***.***> wrote: I've merged it in. Thanks all! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1190 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCILKH5A5WUYR3QZQYVFSLSCZDX3ANCNFSM4P24ZI3A> .

[DavAug]

Thank you Ben and Michael. I've merged it in. Thanks all! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#1190 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AEY2T3QPUQAWGKYNDHIEKODSCZDX3ANCNFSM4P24ZI3A>.