TVB-3038 Correction of JR sigmoidal coupling to match paper and documentation

[dionperd]

A small PR correcting the JR sigmoidal coupling to match the model's simgoidal activation function, as well, as the original Sanz-Leon et al paper on TVB models.

In particular, there was a confusion between parameter "a" being the scaling inside the sigmoidal for all other sigmoidal coupling models, but not for the JR one, for which it is the parameter "r".

So, I have:

• set default r = 0.56,
• default a = 1.0 like in the rest of the sigmoidal couplings,
• and changed the docstring to use r instead of a.

Moreover, I added the cmin (with default 0.0) in the pre() equation method in order to match the docstring. It was missing before.

Finally, in the JR model there is an unnecessary repetition of the multiplication 2 * nu_max parameter. We should consider to absorb this either in the parameter or as a derived parameter to be initialized once during the model.configure(), in a future PR. Please comment on this prospect.

See in the attached image the difference r = 0.56 against r=1.0 makes. Not big, but it is something to consider, especially how many people use the model without noticing it.

[liadomide]

Could you pls have a look also at the linked unit-tests TestCoupling.test_sigmoidal_jr_coupling ?
It seems to be failing now in Test lib / lib-tests.

[maedoc]

Significant corrections are welcome of course, but keep in mind that the JR implemented in TVB is necessarily an approximation and not exact replica of the original paper: there are extra state variables for the coupling which aren't implemented in TVB. This is something that trips up everyone trying to replicate the paper with TVB.

There are other issues such as noise source which can't be replicated in TVB without the code shared on the mailing list.

Lastly, ideally such changes should be opt out so that a user can get the old version somehow, for reproducibility.

I'm ok with the changes if you are showing they match the JR 1995 behavior more closely as long as

• it's documented somewhere e.g. a notebook + change log
• the tests are passing

[dionperd]

Significant corrections are welcome of course, but keep in mind that the JR implemented in TVB is necessarily an approximation and not exact replica of the original paper: there are extra state variables for the coupling which aren't implemented in TVB. This is something that trips up everyone trying to replicate the paper with TVB.

There are other issues such as noise source which can't be replicated in TVB without the code shared on the mailing list.

Lastly, ideally such changes should be opt out so that a user can get the old version somehow, for reproducibility.

I'm ok with the changes if you are showing they match the JR 1995 behavior more closely as long as

• it's documented somewhere e.g. a notebook + change log
• the tests are passing

We didn't suggest this change to better reproduce the original JR paper, but to solve the descrepencies:

• between the sigmoidal used in the JR region model and the JR coupling,
• betweem the Sanz Leon et al paper introducing the math of TVB, and the current implementation of the JR coupling. If you see equations 4.10 and 4.11 in that paper, it is made explicit that the sigmoidals for the JR region model and the JR coupling are the same functions, and then, in Table 13 the parameter for the sigmoidal steepness ("rho_2") is given as 0.56.

We have users that haven't noticed this and being using JR model assuming that the default parameters of the JR coupling and the JR model match, which is not the case, though, probably due to some error when coding for coupling in TVB.

As long as the name of this coupling model includes "JR", i.e., it is not just a sigmoidal coupling but it is meant to be used in conjuction with the JR region model, the default parameters should match.

I don't know whether some documentation is needed for this, as it is the behavior that should have already been present.

I also don't know what is a "change_log".

However, I agree that users should be notified about this change somehow, although for most of them it might be an unpleasant notification as I guess that most haven't noticed that they have been using different sigmoidals for within and among regions' activations!..

I think I have corrected the corresponding test as well.

[liadomide]

I can not comment on the usefulness of the change from a scientific pov, but to provide traceability:
I just added one task in Jira https://req.thevirtualbrain.org/browse/TVB-3038.
This should immediately appear in the detailed release notes for our next release.
I also added one line in the RELEASE_NOTES (where only the more notable changes are mentioned).

[dionperd]

Shall we proceed to merge this?!