Neural mass housekeeping

[agchesebro]

@MasonProtter @helmutstrey cleanup for the VdP we were discussing. I've included an initial stab at what a wrapper function would look like for the split masses (ODE vs SDE). @helmutstrey we should also look at the desired frequency because the default parameters seem a bit high for milliseconds (oscillations are on the order of 100Hz with default params).

[MasonProtter]

Looks like there's an old test here:

Neuroblox.jl/test/components.jl

Lines 278 to 284 in b051eac

	@testset "Van der Pol" begin
	@named VdP = van_der_pol()
	prob_vdp = SDEProblem(complete(VdP),[0.1,0.1],[0.0, 20.0],[])
	sol = solve(prob_vdp,EM(),dt=0.1)
	@test sol.retcode == SciMLBase.ReturnCode.Success
	end

that needs to be removed

[agchesebro]

Oops I think the new Kuramoto split is breaking GraphDynamics

[MasonProtter]

I'm trying to accomodate it now, but I'm getting a very strange disagreement between MTK and GD for the stochastic version that I havent' figured out quite yet.

[MasonProtter]

Okay, I was tearing out my hair trying to figure out what was going on here. Turns out what happened was that this method (I ended up realizing this was the problem while thinking about it in the shower):

Neuroblox.jl/src/blox/connections.jl

Lines 488 to 504 in 56a18fe

	function Connector(
	blox_src::KuramotoOscillator,
	blox_dest::KuramotoOscillator;
	kwargs...
	)
	sys_src = get_namespaced_sys(blox_src)
	sys_dest = get_namespaced_sys(blox_dest)
	w = generate_weight_param(blox_src, blox_dest; kwargs...)
	xₒ = only(outputs(blox_src; namespaced=true))
	xᵢ = only(outputs(blox_dest; namespaced=true)) #needed because this is also the θ term of the block receiving the connection
	eq = sys_dest.jcn ~ w*sin(xₒ - xᵢ)
	return Connector(nameof(sys_src), nameof(sys_dest); equation=eq, weight=w)
	end

was suddenly no longer getting hit by the new KuramotoOscillatorNoise type, and it was instead hitting the generic connection rule for NeuralMassBlox, giving silently incorrect results. I'm point this out not to blame you Anthony, but to point out that this is exactly why we need real tests. Subtle changes like this can suddenly cause blox to give incorrect results, but not throw errors. This could end up going unnoticed for a long time.

The only reason I caught this was that I luckily remembered to do the connection rules for both types in GraphDynamics, but I could have very easily also forgotten to do that, and then if I did, we'd both have incorrect results that agree!

I'm just finishing up my changes for the GraphDynamics side and then I'll push them to this branch.

[MasonProtter]

One other thing, this is technically a breaking change, so we'll need to bump to Neuroblox v0.6.0 if we do this.

Edit: no longer relevant due to #532

[agchesebro]

Ah that is bad. I'm also unhappy in general with this dispatch on the functions as constructors because it's going to be a pretty severe departure from how we've been doing block creation (normally the constructor has the same name as the block). I don't have a great solution for that though, other than holding off on merging this in right away. Sorry for the inconvenience though! Agree that better tests are really becoming necessary (we're working through another headache on the CS model for what's likely a similar compatibility issue).

[harisorgn]

this is exactly why we need real tests

Agreed, we could always use more tests. One issue is that there is no ground truth data for every combination of Blox (there might not even be for a graph of only one type of Blox), so we can't always do the right thing for correctness. I'd say it's safe to assume that it won't exist most of the time, outside of paper figures. So maybe as a middle ground we can add equations checks. Not sure how equality checks for Symbolics expressions works, e.g. if rearranging the same terms in two expressions still make isequal(expr1, expr2) # True , but something like this could be useful. We can fetch the connection_equations and compare them to some hand-written ones (easier to get from a paper than simulation data).