translationalneuromodeling / tapas

TAPAS - Translational Algorithms for Psychiatry-Advancing Science
https://translationalneuromodeling.github.io/tapas/
GNU General Public License v3.0
211 stars 88 forks source link

Bug in HGF softmax_binary_sim.m? #48

Closed nbuergi closed 3 years ago

nbuergi commented 5 years ago

Hi there,

I'm currently playing around with simulated data with the HGF and noticed - what I think is - a bug in one of the scripts. Since I'm new to modelling and programming, though, I might just have misunderstood or overlooked something, so please correct me if I'm wrong.

While tapas_softmax_binary is equipped to take into account reward values via a second and third column in the input u, the corresponding simulation script tapas_softmax_binary_sim doesn't seem to actually take them into account (line 24). Also, the if-condition on line 15 seems to be based on the wrong variable (e.g. since mu1hat is defined as infStates(:,1,1), the statement size(mu1hat,2) == 1 will always be true).

Original code:

if size(mu1hat,2) == 1
    if ~any(mu1hat<0) && ~any(mu1hat>1)
        % Apply the logistic sigmoid to the inferred states
        prob = tapas_sgm(be.*(2.*mu1hat-1),1);
    else
        error('tapas:hgf:SoftMaxBinary:InfStatesIncompatible', 'infStates incompatible with tapas_softmax_binary observation model.')
    end
else
    % Apply the logistic sigmoid to the inferred states
    prob = tapas_sgm(be.*(mu1hat(:,1)-mu1hat(:,2)),1);
end

What I would suggest instead:

if size(r.u,2) == 1  %%<<<<<-------------Replaced mu1hat with r.u, as presumably should be
    if ~any(mu1hat<0) && ~any(mu1hat>1)
        % Apply the logistic sigmoid to the inferred states
        prob = tapas_sgm(be.*(2.*mu1hat-1),1);
    else
        error('tapas:hgf:SoftMaxBinary:InfStatesIncompatible', 'infStates incompatible with tapas_softmax_binary observation model.')
    end
elseif size(r.u,2) == 3
    % Gather the reward values
    r0 = r.u(:,2);
    r1 = r.u(:,3);
    % Apply the logistic sigmoid to the inferred states
    prob = tapas_sgm(be.*(r1.*mu1hat-r0.*(1-mu1hat)),1);  %%<<<<----- added the reward values
end

Does that make sense?

Best, Niklas

chmathys commented 5 years ago

Hi Niklas,

Many thanks for pointing this out. What you’ve found is the historical legacy of using the simulation file in a different use case: two bandits with independent winning probabilities mu1hat(:,1) and mu1hat(:,2) but equal payoff in case of a win. Your suggestion is very welcome because it establishes consistency with the softmax_binary model. The old two-bandit simulation function is not used anymore, as far as I know, and in any case, anyone still attempting to use it after the release will see an error.

Should you like to have credit for your suggestion, you are welcome to fork the TAPAS repository on GitHub and issue pull request. This pull request should contain your suggested alterations to HGF/tapas_sofftmax_binary_sim.m and the addition of your name to the Contributor Licence Agreement

https://github.com/translationalneuromodeling/tapas/blob/master/Contributor-License-Agreement.md

according to

https://github.com/translationalneuromodeling/tapas/blob/master/CONTRIBUTING.md

If all this seems too complicated to you, I’m happy just to implement your suggestion and we’d leave it at that. Please tell me how you’d like to proceed.

Best wishes, Christoph

On 27 February 2019 at 4:01:56 pm, nbuergi (notifications@github.com) wrote:

Hi there,

I'm currently playing around with simulated data with the HGF and noticed - what I think is - a bug in one of the scripts. Since I'm new to modelling and programming, though, I might just have misunderstood or overlooked something, so please correct me if I'm wrong.

While tapas_softmax_binary is equipped to take into account reward values via a second and third column in the input u, the corresponding simulation script tapas_softmax_binary_sim doesn't seem to actually take them into account (line 24). Also, the if-condition on line 15 seems to be based on the wrong variable (e.g. since mu1hat is defined as infStates(:,1,1), the statement size(mu1hat,2) == 1 will always be true).

Original code: if size(mu1hat,2) == 1 if ~any(mu1hat<0) && ~any(mu1hat>1) % Apply the logistic sigmoid to the inferred states prob = tapas_sgm(be.(2.mu1hat-1),1); else error('tapas:hgf:SoftMaxBinary:InfStatesIncompatible', 'infStates incompatible with tapas_softmax_binary observation model.') end else % Apply the logistic sigmoid to the inferred states prob = tapas_sgm(be.*(mu1hat(:,1)-mu1hat(:,2)),1); end

What I would suggest instead: if size(r.u,2) == 1 %%<<<<<-------------Replaced mu1hat with r.u, as presumably should be if ~any(mu1hat<0) && ~any(mu1hat>1) % Apply the logistic sigmoid to the inferred states prob = tapas_sgm(be.(2.mu1hat-1),1); else error('tapas:hgf:SoftMaxBinary:InfStatesIncompatible', 'infStates incompatible with tapas_softmax_binary observation model.') end elseif size(r.u,2) == 3 % Gather the reward values r0 = r.u(:,2); r1 = r.u(:,3); % Apply the logistic sigmoid to the inferred states prob = tapas_sgm(be.(r1.mu1hat-r0.*(1-mu1hat)),1); %%<<<<----- added the reward values end

Does that make sense?

Best, Niklas

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/translationalneuromodeling/tapas/issues/48, or mute the thread https://github.com/notifications/unsubscribe-auth/AS6G_dEiVTHwcsRnfUnI7BgQJeEEVLvAks5vRp3jgaJpZM4bUuzd .

nbuergi commented 5 years ago

Hi Christoph,

Thank you for your kind reply and elucidation. I'm glad I am able to contribute something, as I appreciate the toolbox a lot.

I'll happily issue a pull request. Currently I'm on vacation, but I'll do it as soon as I get back home.

Best, Niklas