Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Starting the monty section #6
Starting the monty section #6
Changes from 15 commits
8fd6b67
94d487f
ba7c859
995024d
69c1611
a7bebc3
a2c25f6
44f526f
6685184
de186bd
89d33e1
8048ac1
cde7aab
6343ebf
0fff7b2
57ce74b
87a98a9
8a501b1
c9a44a3
4a13741
99e02d2
1b09bf5
093f609
737b139
211104d
83a5ca7
c4c0b72
8f5632c
de2fcba
f8543c6
40c0dac
d8b94bc
037dc79
93bba6a
42291a8
eb3a878
af8ce8d
d3be774
7f40429
c5ae337
e0e036a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be basic, not advanced. perhaps just drop this whole file for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed file for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what this file is trying to show, drop for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this into an issue please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced this belongs here, but leave it here for now. It might belong earlier in some general introduction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree - it touches on odin models, that are upwards in the book.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a missing paragraph here outlining what you are trying to cover in this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole section has been reworked, for hopefully adding flow and clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should drop
mcstate
from this, it will just be confusing to peopleThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not feel like it belongs here? Perhaps it belongs in a details section and then linked back to where you want to refer to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been reworked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are just paragraphs, not sections - it would read more smoothly without the headings I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referring to sec-monty-combine here, doesn't seem to exist currently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am sure we could come up with a less abstract example to help people think about this, but this can be done in the next PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there might be a nice biology example with antibody titres and positive vs negative subpopulations - but yes can wait for another iteration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why
l_distribution
- can you use a better name here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to "mixture_model"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is what you need to void vectrisation (plus some consistency in spacing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@richfitz I need to use Vectorize() here to pass multiple value of the argument - let me know if a better interface can be shown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above. You will need to pass a 1-row matrix in to vectorise , so
Please do not use
$density
in docsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how to make this work. The following code:
fn <- function(l, p, m1, m2) { log(p * dnorm(l, mean = m1) + (1 - p) * dnorm(l, mean = m2)) } l_distribution <- monty_model_function(fn, fixed = list(p = 0.75, m1 = 3, m2 = 7), allow_multiple_parameters = TRUE) l <- matrix(seq(from = 0, to = 10, by = 0.1), 1) monty_model_density(l_distribution, l)
Return the following error,
Error in
packer$unpack(): ! Can't unpack a matrix where the unpacker uses 'fixed' Run
rlang::last_trace()to see where the error occurred.
that I interpret as a conflict with having some parameters set as fixed. I could pass a 101x4 instead with the fixed argument like this (that works fine),
`l_distribution <- monty_model_function(fn,
allow_multiple_parameters = TRUE)
l <- matrix(c(seq(from = 0, to = 10, by = 0.1), rep(0.75,101), rep(3,101), rep(7,101)),4)
monty_model_density(l_distribution, l)`
but then it makes things quite more complex to understand and it's not the same as the monty model has then 4 "parameters" (with a mixed interpretation, as one is the actual sampling variable of interest, and the others are parameters of the distributions in the statistical sense) instead of one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
People may get confused with this being
matrix(1)
(it's a 1x1 matrix with a value of 1). If you usedmatrix(0.5)
(say) then it would be easier to talk about (a 1x1 matrix with a variance in the proposal of 0.5)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Changed to a variance of 2 - not much of a difference in sampling performance, with proposal steps on average sqrt(2) bigger.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These run sequentially, not simultaneously
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section feels like it is now in the wrong place