GratingSpecification does not pass trials to CreateGratingTrial in latest versions of Bonsai

[banchan86]

While going through the BonVision docs to reproduce the examples I noticed this bug which seems to have only popped up recently when using the latest versions of Bonsai (I tested it in Bonsai 2.8.0 and 2.8.3). The trials specified in GratingSpecification does not get passed on to CreateGratingTrial, GratingSequence simply reproduces what is defined in CreateGratingTrial .

To reproduce, follow the first example on this page
https://bonvision.github.io/pages/08-GratingSeries/

This issue is also referenced in
https://github.com/orgs/bonsai-rx/discussions/1684

The 2AFC demo in Bonvision examples repo no longer works because of this
https://github.com/bonvision/examples/tree/master/Demos/OriDiscrimination

Workflow attached that reproduces this bug:
grating-stimuli.zip

[banchan86]

Additional bug report by a different user: https://github.com/orgs/bonsai-rx/discussions/2092

[glopesdev]

The 2AFC demo in Bonvision examples repo no longer works because of this
https://github.com/bonvision/examples/tree/master/Demos/OriDiscrimination

@banchan86 I think there may be more than one issue at play here. The orientation discrimination demo doesn't actually use CreateGratingTrial at all. I was debugging it and the issue itself stems from a bug in the EnsureGratingParameters group node where there is an unnecessary DegreeToRadian conversion (the effect of the extra conversion means that you get such a small angle that perceptually there is no difference).

The GratingsSpecification and GratingParameters classes now operate directly on radians, and it seems that this particular example was not updated to reflect this.

I will have a look at the rest of the issues, thanks for looking into this.

[glopesdev]

@banchan86 I found the issue, it is actually pretty wild and possibly deserving of a note for node implementers in the docs or design guidelines somewhere.

The cause of the breaking change is the following PR (see also associated issue discussion as it is much less technical):

• Avoid permissive exclusion of method overloads declared in base type bonsai-rx/bonsai#1512
• Overload resolution of virtual methods is overly restrictive bonsai-rx/bonsai#1502

The reason why CreateGratingTrial suffers from this stems from the way its Process functions have been declared (including only the header declarations below for clarity):

[Description("Creates a sequence of grating trials used for stimulus presentation.")]
public class CreateGratingTrial : Transform<GratingParameters, GratingTrial>
{
    public IObservable<GratingTrial> Process<TSource>(IObservable<TSource> source)
    {
    }
    
    public override IObservable<GratingTrial> Process(IObservable<GratingParameters> source);
    {
    }
}

Long story short, the old Bonsai compiler was not compliant with C# overload resolution rules. We fixed it, which means that now, other things being equal, the override overloads are always less preferred than non-override ones (because technically they come from the base class, not the current class).

The way CreateGratingTrial should have been declared is either:

1. make the first (more general) method the override;
2. avoid base classes by not inheriting from Transform at all and simply declare the Process overloads at the same level (that way the more specific resolution rules apply naturally).

As a general rule I have been recommending more and more that developers prefer 2) whenever possible, unless there is an established class hierarchy (which usually then provides a more general overload as the base method.

Ultimately it boils down to the nuances of the compiler behavior, and we decided we should align strictly to the C# overload resolution rules, otherwise writing c# code and bonsai code would lead to different behaviors at runtime (which was happening before).

Indeed your recommendation is correct to avoid using CreateGratingTrial for now. I will prepare a PR to fix this.