Skip to content
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

uncommented multisession fixed.X0 #84

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

JanGrohn
Copy link

You might have accidentally left fixing X0 across sessions commented out when debugging? When I comment the two lines back in it works again.

@lionel-rigoux
Copy link
Member

Hi Jan,

Thanks for reaching out!

Leaving the "fixing X0 across multisession" commented is voluntary. I understand that it looks like mistake, that why this part of the code was planned to be cleaned out completely in the next release (see the fix-MFX-multisession branch).
The problem is that I could not find a simple way to properly implement this feature. And as there are relatively few use cases for it, I decided that the effort was not worth it. In fact, I would be very curious to know what you are using it for!?

I see you pushed two fixes and mentioned it then behaved as you expected. From what I see, you found a way to simulate a deterministic system with a fixed X0 across sessions, which is nice. However, be careful that this will work only for 1) simulating data and 2) deterministic systems. If the second point is not so important (although it should however be dealt with properly, eg. by throwing an error when trying to fix X0 on a stochastic system), the first one is more critical and can yield pernicious effects. In particular, the code implemented in your branch will not infer correctly on the value of X0, ie. your posterior will be wrong, even if everything seems to run correctly.

If you want to get the "fixing X0" feature work also for inversion (ie with VBA_NLStateSpaceModel), you would need to alter VBA_IX0 to modify the variational scheme to take into account the multiple instances of X0 in the time series, not just the first one as currently implemented. If you're proficient with variational Bayes algorithms, this would require some work but nothing unachievable. I'd be happy to help you implement this feature if you're willing, but I won't have much time as I would like to finish merging the other branches first...

@JanGrohn
Copy link
Author

JanGrohn commented Jun 9, 2020

Hi Lionel,

Thanks for your reply! You're right, I was only thinking about a deterministic system. I'm fitting data from a learning task where participants complete multiple blocks and there's no particular reason why X0 should differ between blocks, which is why I'm fixing it.

Thanks also for pointing out that my 'fix' doesn't work for inverting a deterministic system; I wasn't aware of that. I think the reason I, perhaps naively, assumed it would work was because for the kind of data I'm fitting VBA_IX0 actually doesn't get called--if I understand correctly Theta and X0 get embedded in Phi, which is then handled by VBA_Iphi. Because VBA_Iphi calls VBA_odeLim, which I modified, I assumed it could also infer the posterior for X0. Where am I getting things wrong? Is VBA_odeLim really only used for simulating data and not for inverting it?

I've never implemented a variational Bayes algorithm but if you let me know what function to have a look at I can give it a go.

@lionel-rigoux
Copy link
Member

Oh, you're absolutely right, sorry. Sleep deprivation is not helping me...
For a deterministic system VBA_Iphi takes care of everything and inversion should run correctly, as long as the derivatives returned by VBA_odeLim are properly accounting for the change.
I think your solution might be an elegant fix indeed! I however suspect that there might be some issue with the derivatives, dxdx0 in particular: the initial condition is now the beginning of a block, not simply the first timepoint. Did you check that your derivatives are correct at the session switches (using options.checkGrads)?
Would you mind putting some comments in your code? It would be easier for me to go through it and check that I'm not missing (again) something important.

@JanGrohn
Copy link
Author

JanGrohn commented Jun 9, 2020

That's great to hear! You're right, there was an issue with dxdx0 but I think that's easily fixed by setting dxdx0 = dF_dX; whenever a new block starts? At least for the model I was playing around with it gives me the correct derivatives now.

I've added that change, cleaned up my code, and wrote a couple of comments. I hope you're in bed already and only seeing this tomorrow when you've gotten enough sleep haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants