wsphillips / Conductor.jl

Choo-choo
MIT License
60 stars 12 forks source link

using native unit checks from MTK #44

Closed wsphillips closed 1 year ago

wsphillips commented 1 year ago

fixes #5

Deprecates the use of ConductorUnits in favor of the unit metadata and validation baked into ModelingToolkit.jl

The setup for channel rates are more involved now, since we have to do multiple overloads. This should get fixed with a macro that automatically does the required registration and overloading automatically.

wsphillips commented 1 year ago

unit tests and doc examples need updating

wsphillips commented 1 year ago

Edit: MTK has been patched so that we can disable the standard unit checking when lowering to ODESystem. This means we can use the native units metadata in MTK and do specialized unit validation ourselves, without having to commit to blanket (i.e. all equations) validation. The latest commits here take advantage of the changes on MTK master and will get merged soon as a new version is tagged upstream.

Native MTK unit checking won't work unless we require users to first write all of their rate equations for ion channels as Julia functions first then @register_symbolics, and overload ModelingToolkit.get_unit. This is because there is no symbolic/metadata annotation for ignoring units on certain equations (i.e. unitless gating variables, which are a function of voltage and time). Registering a function also consequently disables tracing, so users may need to write the Symbolics.derivative methods, too. Automating this for the user from symbolic equations at runtime inevitably involves expensive calls to eval.

I've submitted a PR upstream with a fix that could work.

If that's not approved, we could either: 1) Abandon native MTK unit checking and revert to using our own parallel implementation (e.g. ConductorUnits) 2) Use MTK's unit metadata, but disable system checks when converting to ODESystem 3) require users to define their channel rates separately in something that can be precompiled.

wsphillips commented 1 year ago

This will definitely break until the next version of MTK gets tagged. (master branch has fixes that enable latest commits here)