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

Connective fields #4

Merged
merged 7 commits into from
May 29, 2021
Merged

Connective fields #4

merged 7 commits into from
May 29, 2021

Conversation

N-HEDGER
Copy link
Collaborator

Here I have written some initial code to start incorporating connective fields into prfpy. This is based on previous discussions with TK and MA. You can see test/develop/CF_progress for a demo.

Summary (also detailed in commit messages).

  1. As suggested, I have now separated out the pycortex-related stuff required for making the subsurfaces and distance matrices. This now sits in utils.py. There is a 'subsurface' class that can make the source subsurfaces for a pycortex subject. All of my pycortex-based visualisation stuff has also now gone.

  2. As suggested, the distance matrices are now padded, so that distances in the opposite hemisphere are coded as np.inf. This prevents us having to treat the hemispheres separately, as was the case in my previous effort.

  3. I created new stimulus and model classes, these are significantly more minimal than they were before and are structured similarly to the existing classes. The CFStimulus class only requires i) the data, ii) the vertex indices defining the subsurface and iii) the distance X distance matrices. Therefore, it is up to the user whether they want to use the 'subsurface' class in utils.py to provide this information or not. In principle, all the relevant information that is provided to the CFstimulus class could be created outside of pycortex - there is no requirement to use it.

  4. At the fitting stage, things start to diverge a little. As discussed before, we may not always want to do a full GLM-based fitting for the CF models - we may want to use a fast method based on the dot product of z-scored data as used in Tomas' paper. Therefore, I include a quick_grid_fit method that does all the fitting via np.tensordot. This returns an array called quick_gridsearch_params to differentiate them from the gridsearch_params produced by grid_fit.

  5. Currently, as far as I can tell, the current cross validation methods only work on the iterative search params - but if you want to use this 'quick' method, you would want to do the cross validation instead based on the gridsearch params. Hence, I have included a 'quick_xval' method.

  6. In returning the best fitting parameters, note that the vertex centres should be ints - and keeping them in an array with the other parameters seems to force them into a float. Therefore, I additionally return the vertex_centres as a vector of ints.

To do list

  1. Currently, I have not yet implemented a way in which the gridsearch params are fed into the iterative fitting stage. As we discussed before - we don't want to optimise the vertex centres - we only really want to optimise sigma. We should discuss how the current iterative fit methods allow us to ignore some parameters, or whether we would have to write a completely new method that ignores the vertex centres. One method of doing this would be to remove the vertex centres from the starting params, convert them into a dictionary and enter this as one of args to the iterative_fit method. Then when it comes to the error function (below), my understanding is it will use the args (vertex centre) to generate the model prediction, but it wont try and optimise args, only parameters
 np.nan_to_num(np.sum((data - objective_function(*list(parameters), **args))**2), nan=1)

One other alternative we discussed was to somehow use iterate over the distance matrix instead. At any rate, it is worth discussing the best way to do this.

  1. One problem could be that creating a new function for the 'quick' fitting may not fit stylistically. Instead, it might be better to have one function for grid fitting, that has an argument that determines whether whether quick or GLM based fitting is performed. i.e.
gf.grid_fit(sigmas,method='GLM')
gf.grid_fit(sigmas,method='quick'')

N-HEDGER added 7 commits January 19, 2021 17:00
I seperated out the surface-based stuff into Utils.py. There is now a 'subsurface' class that does all the pycortex based stuff. This does all the donkey work
that is needed before defining a 'stimulus'. see test/develop/CF_progress.

I also inserted a command to pad the subsurfaces for each hemisphere so that they can be combined and will have the same shape as the design matrix.

The surfaces are removed after they are no longer needed (after computing the distance matrices).
I created a model and stimulus class for gaussian CF. It generates grid and arbitrary predictions

I also altered the padding of the distance matrices as I did this incorrectly. Now distances to the opposite hemisphere are treated as infinite values.
Created a fit class for the CF model. Contains the original grid fitting method (using a regression and paralell batch processing) and also a
 quick_grid_fit method that uses the zscored dot product method.
I have now written everything so that the 'fast' grid fitting (dot product method) can be done with cross-validation. See the CF_progress.ipynb for an illustration. I also removed all the plotting functions from the .utils file and provided fuller annotations.
@tknapen tknapen marked this pull request as ready for review April 7, 2021 12:25
@marcoaqil
Copy link
Collaborator

Hi Nick! Really awesome stuff here, sorry for the late reply. One way to keep some parameters fixed in iterative fit while fitting the others is to use the Bounds argument of python optimizers. When an optimizer is given a bound for a parameter where lower and upper bounds are identical like (1,1), it will just keep that parameter fixed to 1. I do this in the context of some pRF models. In my head, that is stylistically better than removing some parameters from the array, but in practice the latter might work better. Whatever works works :)

Let know when you are done coding and testing this branch, and we should indeed start merging it into the master.

@marcoaqil
Copy link
Collaborator

Merged today!
on the to do list, as from my comment above:
Because it is something that is useful in a variety of situations, I will implement myself ASAP a minimal way to keep certain parameters fixed in iterative fitting to a specific value, and only fit others, without changing the structure of fitting params array (which could create problems later on) or using additional non-array structures (dictionaries). See the relevant issue that I opened

marcoaqil added a commit that referenced this pull request Dec 12, 2022
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.

None yet

2 participants