klabhub / neurostim

Design and run visual neuroscience experiments using Matlab and the Psychophysics Toolbox.
MIT License
4 stars 3 forks source link

optional parameters for noiseclut.reconstructStimulus #208

Open dshimaoka opened 1 year ago

dshimaoka commented 1 year ago

Added new optional parameters (xPixRange, yPixRange) to limit reconstruction range. These are useful to prevent out-of-memory error when #pixels are too large.

adammorrissirrommada commented 1 year ago

Hi @dshimaoka . This is a very good thing to include. Thanks. I haven't yet reviewed the details of the implementation, but can I suggest you make the input arguments in ns-coordinates (i.e. in the same units as the stimulus' position and width)? Internally, those would be converted to pixels with cic.physical2pixel etc. Pay particular attention to the y-axis, where negative and positive values correspond to below and above the horizontal meridian, respectively. Though I think the cic converter will sort all that out for you.

In general, the philosophy of ns is to use ns' "physical" screen coordinates (e.g. degrees, cms, etc. whatever is specified for c.screen.width etc) for spatial stuff on the "outside" (e.g. in experiment scripts) at all times, except in special circumstances where pixels are fundamental to a stimulus.

Another suggestion would be to have the input argument as a rect vector (four values) consistent with the broader behaviour of psychtoolbox.

cnuahs commented 1 year ago

The issue I see here is that it requires noiseclut to know the physical coordinates of each randel. Maybe I'm wrong but I don't think noiseclut currently has that information, or any way to get it...

cnuahs commented 1 year ago

At the level of noiseclut, I think the best you can do is limit the reconstruction, or rather, the values returned, to a subset of randels. In which case, the concept of a rectangle (in pixels or physical coordinates) makes little sense.

Maybe noiseclut should just accept an optional "mask" argument that masks off the randels to return CLUT entries for. It would then be up the the child classes to [optionally] overload the reconstruction method and supply an appropriate mask, using whatever nomenclature makes sense given their context and parameterisation.

dshimaoka commented 1 year ago

Tried pushing another one that reflected Adam's suggestion:

What is done inside is simply limiting values returned as Shaun suggested. Not sophisticated, yet still helps to prevent an out-of-memory error.

cnuahs commented 1 year ago

I made this comment on the commit itself, but not sure where that actually went:

This doesn't seem right. This (9de00d0) seems to be assuming that randels are arranged in a regular grid. That would be fine in say neurostim.stimuli.noisegrid (although I think you'd have to account for any rotation also) but neurostim.stimuli.noiseclut is much more flexible than that and no assumptions are made about the spatial arrangement of randels. Randels can be arranged in almost any way. It need not be continuous or even parametric.

I really think this should be handled more generally here as a mask, e.g., a binary vector, nrRandels x 1, that is true for randels you want to return clut vales for. That mask would [optionally] be provided by the concrete child classes in whatever form makes sense for them, e.g., a regular rectangular grid (c.f. noiesgrid, much like you see here), a polar grid (c.f. noiseradialgrid) a hexagonal grid (c.f., noisehexgrid), or even no grid at all if that makes sense for whatever stimulus you're talking about.

dshimaoka commented 1 year ago

In the revised version, the optional input rect is replaced with a more primitive one randelMask, which works whatever the arrangements of randels as: clutVals{i} = clutVals{i}(:,randelMask,:); The conversion from rect to randelMask will be done somewhere else.

dshimaoka commented 1 year ago

In the latest push, I tried implementing "overloading the reconstructStimulus() method in neurostim.stimulus.noisegrid ". Also in noiseClut.reconstructStimulus, the 2nd output ixImage is now also limited by rect. Confirmed the two outputs with noisegrid and noiseradialgrid. Third-time lucky?