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

DBS protocol #481

Merged
merged 11 commits into from
Nov 8, 2024
Merged

DBS protocol #481

merged 11 commits into from
Nov 8, 2024

Conversation

gabrevaya
Copy link
Contributor

Allows setting up DBS protocols similar to those empirically used in clinical studies to obtain ERNAs.

@gabrevaya gabrevaya requested a review from harisorgn November 4, 2024 13:52
@gabrevaya
Copy link
Contributor Author

(I cancelled the last checks because I had only added some docstrings)

offset=0.0,
start_time=0.0,
smooth=1e-4
p = paramscoping(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These parameters are set as tunable=true in paramscoping, was this intended for the current case? E.g. do you expect to fit such parameters in an inverse problem setting?

  • If no, then I'd say just define your parameters explicitly here with tunable=false to avoid having them included in optimization/fitting problems.
  • If yes, then maybe consider moving the paramscoping call at the beginning of the function, in case any of these parameters are Nums instead of numbers, so that the correctly scoped parameters will be used in your stimulus function. In a tunable=true setting it could be the case that multiple sources share the same parameters, so the input types could be the same Nums among multiple DBS sources. In this case you would need to overwrite the input parameters with the return of paramscoping e.g. like here
    p = paramscoping=τ, H=H, λ=λ, r=r)
    τ, H, λ, r = p

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticing this! I'm not expecting to fit these parameters, at least for now. Maybe in the future we could fit them, but we can change it to tunable=false for now and modify it in the future if needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ye I wasn't expecting to fit source parameters either to be honest. Usually these are known.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For setting parameters as non-tunable, which option would you suggest?

  1. Use paramscoping and then untune all parameters with untune!
  2. Manually set each parameter as @parameters $p = p [tunable=false]
  3. Add a tunable flag to paramscoping:
function paramscoping(;tunable=true, kwargs...)
    paramlist = []
    for (kw, v) in kwargs
        if v isa Num
            paramlist = vcat(paramlist, ParentScope(v))
        else
            paramlist = vcat(paramlist, @parameters $kw = v [tunable=tunable])
        end
    end
    return paramlist
end

I would go with option 3).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, go for it.

@harisorgn
Copy link
Member

Besides the one comment above, could you add some tests for the new source & protocol?

@gabrevaya
Copy link
Contributor Author

On a different topic, regarding conventions, currently, I call DBS the constructor of the simple stimulus and protocol_dbs the constructor of the protocol. However, I guess it should be more appropriate to use Protocol_DBS or ProtocolDBS, right?

Also, since Neuroblox's unit of time is ms, I'm using frequency here in units of 1/ms = kHz. However, I wonder if we should accept Hz as input and handle the conversion internally. This could be more user-friendly since users will be more likely to think in Hz, especially in the case of DBS, where frequencies are $\sim$ 130 Hz.

@harisorgn
Copy link
Member

harisorgn commented Nov 5, 2024

However, I guess it should be more appropriate to use Protocol_DBS or ProtocolDBS, right?

No strong feelings about it. Technically it is a constructor dispatch but I guess you use a different name (other than DBS) to keep the arguments as keyword for user-friendliness. I'd say ProtocolDBS between the two.

However, I wonder if we should accept Hz as input and handle the conversion internally.

Sure as long as you document that in the docstring. We should start using a units package soon.

@gabrevaya gabrevaya merged commit 6f59b05 into master Nov 8, 2024
6 checks passed
@gabrevaya gabrevaya deleted the DBS_protocol branch November 8, 2024 13:56
david-hofmann pushed a commit that referenced this pull request Nov 11, 2024
* initial DBS protocol for getting ERNAs

* unify DBS and DBSProtocol into single type

* add DBS protocol example

* add docstrings for `DBS` and `protocol_dbs` constructors

* add a tunable option to `paramscoping` function

* set DBS and protocol_dbs parameters as not tunable

* change constructor name to ProtocolDBS

* change input frequency units to Hz

* add tests for ProtocolDBS

* adjust `frequency` in `compute_transition_times`

---------

Co-authored-by: haris organtzidis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants