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

extended and multisource #28

Closed
lionel-rigoux opened this issue Jun 16, 2017 · 14 comments
Closed

extended and multisource #28

lionel-rigoux opened this issue Jun 16, 2017 · 14 comments

Comments

@lionel-rigoux
Copy link
Member

There are some redundancies in the core and display code to process differentially single or multi-sources data. Part of the switches between these two trails relies on the number of sources, some on an "extended" flag in the options structure. The latter was introduced to test the equivalence of the processing of single source models accross the two versions of codes. Therefore, this flag is not reliable to indicate the number of sources and only add unecessary redundancy in the instructions workflow.
I think the redundancy should be reduced to its minimum and the "extended" flag should be removed. In particular, this concerns the following functions:

  • VBA_initDisplay.m / VBA_initDisplay_extended.m
  • VBA_updateDisplay.m / VBA_updateDisplay_extended.m

Part of the redundancy is however required has the treatment of stochastic and multisources models cannot be yet fused (cf. VBA_Iphi.m)

@jdaunize
Copy link
Contributor

Hey Lionel,
I agree. I'll remove the 'extended' field where I know it is, namely:

  • in VBA_check
  • in display functions VBA_initDisplay and VBA_updateDisplay (where I'll simply check the number of sources).
    Any other idea?
    J.

@jdaunize
Copy link
Contributor

Oh and also in VBA_GN...

@lionel-rigoux
Copy link
Member Author

lionel-rigoux commented Jun 16, 2017

I don't have it all in mind, but an advanced search for text in matlab should be able to point to all occurences of the flag.

Concerning the display, I will take care of merging (respectively for init and update) the two versions into a unique versatile function. Having said that, you could still replace the extended flag by a source number check, it won't hurt!

@lionel-rigoux
Copy link
Member Author

lionel-rigoux commented Jun 16, 2017

Well, the flag should be in all places the multisource can kick in. I used this flag to extensively check on single source models that the old code (single source) and the new one (intended for multisource but activated by the flag) gave strictly identical results.

@jdaunize
Copy link
Contributor

So i've been replacing the 'extended' flag with a check on the number of sources everywhere I've seen it.
I've ran a number of demos, and it all looks fine. I'll commit soon.

@jdaunize
Copy link
Contributor

Re: multi-source display: I'm kind of unhappy here, because it systematically cracks with me.
Besides, I think it should be optimized because the current version will look really crap if the number of sources incerases...

@lionel-rigoux
Copy link
Member Author

What do you mean "cracks"?
I went up to a dozen sources without problems. A scroll appears on the right to navigate through the sources when it doesn't fit the panel. Having said that, I have no strong feeling about this!

@jdaunize
Copy link
Contributor

:)
When I navigate through the tabs, it cracks...

@jdaunize
Copy link
Contributor

Here is a series of screen captures:
bugs_VBA.pptx

The 2 first ones ar eduring the inversion:

    1. the top axes are not well positioned
    1. when I scroll down with the slidebar, the bottom axis has scrambled legend and labels

The 2 last ones are after VBA convergence:

    1. OK
    1. I navigate through the tabs: 'inversion', 'diagnostic' and then... it cracks :(

@lionel-rigoux
Copy link
Member Author

lionel-rigoux commented Jun 16, 2017

Ok I found the problem:

You're using maltab 2013a, I am using the 2016a version. There was a major modification of the graphical engine in the 2014b release. Thoses modifications affect in particular the way "panes" are handled in the hierarchy of graphical obejcts included in the figure. Critically, if you using the wrong system leads to the plots beeing drawn beneath the pane, hence the "white page" effect. In fact the plots are there, hidden behing the white pane...

I applied a patch using a subfunction "getPanes" designed to get the handle to the selected pane irrespective of the Matlab version. Incidently, the patch does not appear in VBA_initDisplay but only in VBA_initDisplay_extended (that's why I hate duplicated code...). Therefore: I have a problem with the stochastic demo but not you, as they rely on VBA_initDisplay which works with the old panes system. You have problem with multisource demo but not I as they rely on VBA_initDisplay_extended which works with the new panes. having said that, I realize that my getPanes trick does not work for Matlab 2013a (but is fine for 2014a).

@lionel-rigoux
Copy link
Member Author

We need to find a better selector to retrieve the handle to the pane correctly irrespective of the Matlab version. Moreover, I really need to merge those display functions as those kind of half bug corrections are quite nasty to identify.

@lionel-rigoux
Copy link
Member Author

lionel-rigoux commented Jun 16, 2017

And the axes ill positioned, it is also related to the change of the status of panel in the graphical hierarchy. The bug was kind of multisource specific because the multisource panels require a scrolling option for the panes that is badly handled in old versions. A quick workaround would be to add an offset depending on the graphical engine version.

@lionel-rigoux
Copy link
Member Author

lionel-rigoux added a commit that referenced this issue Jul 6, 2017
Major changes fo the display system:
- merge extended and historical display function
- solve compatibility issues related to the 2014b graphical motor change
- numerous other bug fixes
- simplify figure identification for in-place redrawing
- cosmetic changes (plot arrangement, color consistency)
- add some documentation
@lionel-rigoux
Copy link
Member Author

Solved by pull request #32

lionel-rigoux added a commit that referenced this issue Jul 7, 2017
* extend check_struct to struct defaults

* enforce stricter checks of user defined model dimension

* fillInPriors makes use of VBA_priors to set default (#26)

* test commit (cosmetic change on VBA_hyperparameter.m)

* cleaning VBA goodness-of-fit summary statistics

WARNING: before, the field .R2 used to report the coef of determination
for continuous data, and the balanced accuracy for binary data. This has
now changed, and the fields fit.R2, fit.acc and fit.bacc always report
the same metric, namely: determination coefficient, classifical accuracy
(NaN for gaussian sources) and balanced classification accuracy  (NaN
for gaussian sources), respectively.

* correcting minor display bug

* minor changes in VBA results reporting

The changes actually deal with multi-source inversion.
NB: this fix does not solve graphical display issues with multi-source
inversion

* another set of bug fixes on VBA graphical results display

NB: still, this does not solve issues when VBA is in multi-source
mode....

* fix minor display bugs for VBA_hyperparameters.m

* bug fix for the zero prior variance case

Previous changes in VBA_prior and VBA_check unfortunately removed the
possibility to fix unknown model parameters to their prior value, which
was done by setting the corresponding prior variance to zero. This has
now been corrected back.

* major update of k-toM and BSL models

- correcting on k-ToM observation and evolution functions
- correcting on k-BSL observation and evolution functions
- updating "unwrap" functions for BSl and k-ToM learners
- adding "meta-ToM" learners (mixture of ToM and BSL experts)
- adding demonstration scripts

* updates on VBA statistical tools

- adding SEM_analysis0
- modifying a few GLM and RFT subfunctions

* a further series of minor bug fixes

- removed the 'extended' field in the options structure, and replaced it
with appropriate checks on the number of sources
- re-introduced params2update substructure after padding priors with
defaults
- overwrite ODE_posterior subfield in VBA_hyperparameters

* bug fixes in sparse inversion

- changed minimal gradient in sparseTransform for numerical convergence

* updates of a few demos

* update on VBA_MFX

- simplify priors checking
- fixed by-passing of VBA initialization
- eyeballing results in demo_MFX

* update VBA_MFX

- simplified priors checking
- fixed by-passing of VBA initialization step

* Update README.md

* Fix bernoulli.m clashing with Sym Toolbox (#30)

The symbolic toolbox has a bernoulli function which clashes with VBA's. This fix renames VBA's bernoulli.m file to VBA_bernoulli.m and updates references to bernoulli.m to reference the new function name.

* update on VBA_classification

- change in summary statistics
- change in results display
[ & minor update of unwrap_KBSLm ...]

* minor change in demos

* Small fix to mediation_contrast (#23)

* Fix for the wrong regression coefficient computation in stats&plots/mediationAnalysis0.m

* in stats/plots/RFT_* I fixed the call to cdf and icdf ('norm',..) to ('norm',..,0,1) because this command gives NaN values with matlab R2016a

* Fixed the numerical issues of 0/0 in RFT_Pval (when normcdf return 0 instead of a tiny number)

* Add nanxxx subfunction with VBA_ prefix, based on nansuite. Closes #31

* move nan subfunctions to avoid reinstall on pull

* update of k-ToM demos

* included commentaries in VBA_getDiagnostics

* accelerate VBA_MFX

bypass volterra kernel estimation within VB meta-loops

* accelerate VBA_hyperparameters

bypass volterra kernels estimation within VB meta-loop

* bug fixes

* a few more bug fixes on RFT (gaussian field)

* Fix display (issues #28 and #29) (#32)

Major changes fo the display system:
- merge extended and historical display function
- solve compatibility issues related to the 2014b graphical motor change
- numerous other bug fixes
- simplify figure identification for in-place redrawing
- cosmetic changes (plot arrangement, color consistency)
- add some documentation

* small trick to reduce demo run time

* add missing spm function

* remove legacy call to simulateNLSS_fb

* fix bug in when VBA_design_efficiency is given non array inputs

* remove dependency on zscore (stats toolbox)

* fix bug in VBA_sample for multinomial data

* add demo testing function

* bug fix on demo_spm_distributed

* fix display/verbose bugs

* remove demo_spm_hrf

no problematic code, no problem :)

* fix minor bug on demo_BSL

* remove dummy demo_test

useless

* remove demo_nullSpace

useless

* remove demo_interaction

useless

* added CDF of noncentral F-distrivution

* Remove external dependencies to fix #33

- replace cdf calls by spm_cdf counterparts
- replace call to external nan* functions by spm couterparts
- add in-house simple  functions for quantile, ncfcdf, kstest
- fix typo and small issues in demos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants