How to handle extensions to EyeTracking neurodata type?

[rly]

The Allen Institute maintains an extension "ndx-ellipse-eye-tracking" that defines two new neurodata types: EllipseEyeTracking and EllipseSeries.

EllipseEyeTracking extends the NWB Core EyeTracking and requires 3 EllipseSeries (named "eye_tracking", "pupil_tracking", "corneal_reflection_tracking") and 1 TimeSeries (named "likely_blink").
EllipseSeries extends the NWB Core SpatialSeries.

The NWB Core EyeTracking schema contains:

groups:
  - neurodata_type_inc: SpatialSeries
    doc: SpatialSeries object containing data measuring direction of gaze.
    quantity: '+'

where quantity: '+' means at least one.

That quantity spec in the EyeTracking schema was changed in NWB 2.9.0 from '*' to '+'. That change makes sense - you should not create an empty EyeTracking object.

However, because EllipseEyeTracking extends EyeTracking, EllipseEyeTracking now also contains the requirement of at least one other SpatialSeries, and previously written data with this extension are no longer valid because they lack an additional SpatialSeries. Even though an EllipseSeries extends SpatialSeries, the 3 EllipseSeries objects already correspond to other requirements of EllipseEyeTracking and cannot be used to satisfy the requirement for at least one SpatialSeries.

To resolve this, I think I should update this extension to make EllipseEyeTracking extend NWBDataInterface instead of EyeTracking. This feels correct to me, but it has the downside that it adds a new wrapper container for eye data. I think that is OK. I am curious to hear what others think about this though.

[oruebel]

However, because EllipseEyeTracking extends EyeTracking, EllipseEyeTracking now also contains the requirement of at least one other SpatialSeries

Should EllibseEyeTracking always contain that fixed set of series ( "eye_tracking", "pupil_tracking", "corneal_reflection_tracking", and "likely_blink"), or would it potentially also want to contain other SpatialSeries? If so, then I am wondering whether it may be simpler to revert this change to the EyeTracking module and have this as a general check in the NWBInspector instead:

That quantity spec in the EyeTracking schema was changed in NWB 2.9.0 from '*' to '+'. That change makes sense - you should not create an empty EyeTracking object.

If EllibseEyeTracking is fixed to those 4 objects, then yes, I think this is the correct approach, because we can't express right now in the schema that EllipseSeries replaces the requirement for SpatialSeries:

To resolve this, I think I should update this extension to make EllipseEyeTracking extend NWBDataInterface instead of EyeTracking. This feels correct to me, but it has the downside that it adds a new wrapper container for eye data.

Either way, EllibseEyeTracking is a new container. The main information that is lost when using NWBDataInterface as a base is that EllibseEyeTracking is no longer a specialization of EyeTracking.

The real underlying issue seems to be that we don't have a way in the schema to indicate that, e.g., the eye_tracking object is a refinement of the parent spec for SpatialSeries. In principle, it satisfies that requirement, but we don't have a way to describe this because all the definitions are for EllipseSeries as substypes of SpatialSeries. However, allowing for describing this case seems tricky. The most explicit approach I think would be to add the option to assign "keys/name" to the spec itself to allow us to explicitly reference which spec is being refined, rather than relying on implicit refinement based on neurodata_type alone. I think this would work, but the effort for adding such a feature I think may be too high and would add more edge-cases.

Long story short, I think your proposed approach to use NWBDataInterface as a base class instead is reasonable.

[rly]

Thank you for the detailed response @oruebel !

Should EllipseEyeTracking always contain that fixed set of series ( "eye_tracking", "pupil_tracking", "corneal_reflection_tracking", and "likely_blink"), or would it potentially also want to contain other SpatialSeries?

This is a lab-specific extension to store the results of a processing tool, so it always contains just that fixed set of series and no others. I expect that future use cases of this extension will not want to contain other series.

The most explicit approach I think would be to add the option to assign "keys/name" to the spec itself to allow us to explicitly reference which spec is being refined, rather than relying on implicit refinement based on neurodata_type alone. I think this would work, but the effort for adding such a feature I think may be too high and would add more edge-cases.

Interesting idea. I like it. I agree that the effort is too high given limited bandwidth right now and risks adding more edge cases.