do not recompute lf if already existing #2339
Conversation
|
You should test whether your modifications do not break anything. When outside the DCCN, please consider testing: test_ft_dipolefitting, test_example_simulate_forward_dipolefit, test_pull1377b When inside the DCCN, please also consider testing: test_bug3119, test_eeglab_ft_integration, test_pull1377b, test_bug2773 Suggested tests outside the DCCN use public data or do not use data. |
There was a problem hiding this comment.
Hmmm, are you sure that this change will work? Also in general?
Specifically, you check for ~lf, but this will throw an error if the variable lf does not exist in the function. This could be the case for instance if someone runs a nonlinear fit only (i.e. without gridsearch).
Also, generically, lf to me seems the forward solution to one of the dipoles in the grid, which is updated in the for-loop across the dipoles in the grid. The lf outside the gridsearch loop is just the leadfield of the last dipole in the grid, and not necessarily the one that gave the optimal solution.
Or am I missing something?
|
You should test whether your modifications do not break anything. When outside the DCCN, please consider testing: test_ft_dipolefitting, test_example_simulate_forward_dipolefit, test_pull1377b When inside the DCCN, please also consider testing: test_bug2773, test_eeglab_ft_integration, test_bug3119, test_pull1377b Suggested tests outside the DCCN use public data or do not use data. |
|
You should test whether your modifications do not break anything. When outside the DCCN, please consider testing: test_ft_dipolefitting, test_pull1377b, test_example_simulate_forward_dipolefit When inside the DCCN, please also consider testing: test_bug2773, test_pull1377b, test_eeglab_ft_integration, test_bug3119 Suggested tests outside the DCCN use public data or do not use data. |
|
you are right, sorry. |
ft_dipolefitting.m
Outdated
| % if there is no leadfield, re-compute it in order to compute the model potential and dipole moment | ||
| lf = ft_compute_leadfield(dip(t).pos, sens, headmodel, leadfieldopt{:}); | ||
| else | ||
| lf = dip.lf; |
There was a problem hiding this comment.
would you need to consider the loop-variable (and also in line 466?)?
|
You should test whether your modifications do not break anything. When outside the DCCN, please consider testing: test_pull1377b, test_example_simulate_forward_dipolefit, test_ft_dipolefitting When inside the DCCN, please also consider testing: test_pull1377b, test_bug2773, test_eeglab_ft_integration, test_bug3119 Suggested tests outside the DCCN use public data or do not use data. |
|
Hi @mcpiastra, have you tried to run the suggested test functions on your end (given the changed code)? Let me know if those functions ran through without error, then I will merge. |
|
You should test whether your modifications do not break anything. When outside the DCCN, please consider testing: test_example_simulate_forward_dipolefit, test_pull1377b, test_ft_dipolefitting When inside the DCCN, please also consider testing: test_bug3119, test_eeglab_ft_integration, test_pull1377b, test_bug2773 Suggested tests outside the DCCN use public data or do not use data. |
|
hi @schoffelen, after the last commit, yes, all the suggested tests run with no error. |
|
Hi @mcpiastra I think that there is not much work to be done on this anymore. In other words, I think it can be merged, but it would be great if this PR also includes a snippet of test code added to a relevant test function (or a new test function altogether). Do you have some code lying around that allows for testing the functionality, or could you write some? |
this relates to the issue #2188
after the dipole scan, the leadfield should be recomputed only if not already existing