-
Notifications
You must be signed in to change notification settings - Fork 11
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
1D gaussian input node implementation (WIP) #129
base: master
Are you sure you want to change the base?
Conversation
Implementation of a 1D Gaussian into ProbabilisticCircuits.jl, missing the update_params function currently.
Is the |
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.
Added comments inline.
|
||
function sample_state(dist::Union{BitsGaussian, Gaussian}, threshold, heap) | ||
# Sample from standard normal | ||
z = randn() |
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.
Threshold already gives you the random number you need, so probably should not sample again. We sample all the randomness we need beforehand, and pass them along to the input nodes, so here threshold is basically sampled from a uniform distribution from [0,1]
. The threhold is basically log of a uniform random variable from [0-1].
I guess for Guassian might need to use the reverse CDF to make it work using threshold. If its not easy to do, maybe we can adjust how randomness is passed to the input nodes.
Here is where we pass the random numbers to input nodes sample_state
https://github.com/Juice-jl/ProbabilisticCircuits.jl/blob/27cb093439c8db5b6e59f75567800ff92d4fffa6/src/queries/sample.jl#L174
And we sample all randomness needed before calling the kernel
https://github.com/Juice-jl/ProbabilisticCircuits.jl/blob/27cb093439c8db5b6e59f75567800ff92d4fffa6/src/queries/sample.jl#L133
src/nodes/gaussian_dist.jl
Outdated
error("Not implemented error: `update_params`, $(typeof(dist))") | ||
#heap_start = dist.heap_start | ||
|
||
#missing_flow = heap[heap_start + GAUSS_HEAP_MISSINGFLOW] |
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.
Here I think we should be very similar to binomial.
- For computing "node_flow" we just add missing_flow to it directly.
- For computing "node_flow * value", if we have missing flow then we add
oldp * missing_flow
to it, the reason is that if we don't observe that guassian the expected value is theoldp
(orold_mu
) in this case.
pseudocount
was basically used to avoid getting 0 probabilities, so its preteding we observed x=i
for every i. For the guassian case I case we would treat it the same as the missing_flows
(or can ignore it for now since not sure if it fits with continous/guassian vairable ) so can first try with pseudocount=0
Overall, I think should look like something like this,
missing_flow = heap[heap_start + GAUSS_HEAP_MISSINGFLOW]
node_flow = heap[heap_start + GAUSS_HEAP_FLOW] + missing_flow + pseudocount
old_mu = heap[heap_start]
new = (heap[heap_start + GAUSS_HEAP_FLOWVALUE] + (missing_flow + pseudocount) * old_mu ) / (node_flow)
Codecov ReportBase: 41.10% // Head: 28.29% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #129 +/- ##
===========================================
- Coverage 41.10% 28.29% -12.81%
===========================================
Files 23 24 +1
Lines 1927 1990 +63
===========================================
- Hits 792 563 -229
- Misses 1135 1427 +292
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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 with the new changes we are almost there.
The main thing is for randomness probably want to do that outside the kernel and initialize with normal random here (basically need to somehow decide to do normal random samples or uniform based on the context)
https://github.com/Juice-jl/ProbabilisticCircuits.jl/blob/27cb093439c8db5b6e59f75567800ff92d4fffa6/src/queries/sample.jl#L133
For bigger benhmarking/testing can probably use Rat-SPN structure with guassian leaves to see if its learning, similar to here (just need to normalize mnist features from UInt8 to floating version).
This PR proposes an implementation of a 1D Gaussian input node (in relation to Issue #128). Currently implemented are some of the new input node code and related IO functions, along with their respective tests.
I'm currently trying to figure out how to implement the 'update_params' function, but I'm a bit confused on how to utilize the missing flow, flow and flow*value to do so. Any tips or feedback on this or any other aspects of the code would be much appreciated.
Other implementational details of note that are open questions are: