Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Can't mix and match BiphasicAxonMapModel #565

Open
mbeyeler opened this issue Oct 7, 2023 · 2 comments
Open

[BUG] Can't mix and match BiphasicAxonMapModel #565

mbeyeler opened this issue Oct 7, 2023 · 2 comments
Labels

Comments

@mbeyeler
Copy link
Member

mbeyeler commented Oct 7, 2023

I have a stimulus made exclusively of well-behaved BiphasicPulseTrain objects and the following model:

model = p2p.models.Model(
    spatial=p2p.models.AxonMapSpatial(n_axons=2000, yrange=(-8, 8), xrange=(-12, 12)),
    temporal=p2p.models.FadingTemporal(tau=200)
).build()
model.predict_percept(implant).play()

It works just fine. But switch out AxonMapSpatial for BiphasicAxonMapSpatial:

model = p2p.models.Model(
    spatial=p2p.models.BiphasicAxonMapSpatial(n_axons=2000, yrange=(-8, 8), xrange=(-12, 12)),
    temporal=p2p.models.FadingTemporal(tau=200)
).build()
model.predict_percept(implant).play()

results in ValueError: Cannot calculate temporal response, because stimulus/percept does not have a time component, which is clearly not true (/models/base.py, line 585)

@mbeyeler mbeyeler added the bug label Oct 7, 2023
@jgranley
Copy link
Member

This is currently by design, but is something that we could consider changing (and that error certainly is not the most helpful).

Right now it doesn't completely make sense to use it with a temporal model, because the biphasic model is already accounting for lots of temporal information (pdur, freq) in its calculations, and its not clear how this should interact with another model that also uses those features.

This is similar to the issue with the Dynaphos model (it really isnt possible to completely separate out temporal and spatial components in either of these).

From the BiphasicAxonMapSpatial docstring:
.. note:: Using this model in combination with a temporal model is not currently supported and will give unexpected results

Should we consider allowing mix and match behavior? This might allow for the desired qualitative effects (e.g. phosphenes that scale with stim params and fade), but doesn't really make sense looking at the theoretic motivation / assumptions behind each model.

@jgranley
Copy link
Member

Also wanted to add that enabling this behavior would not be hard. The bug arises because the BiphasicAxonMapModel is designed to output a singular brightest frame, which then does not work with the temporal model. A quick fix would be to just copy this frame at every point and then use the temporal model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants