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

using native unit checks from MTK #44

Merged
merged 13 commits into from
Sep 2, 2022
Merged

using native unit checks from MTK #44

merged 13 commits into from
Sep 2, 2022

Conversation

wsphillips
Copy link
Owner

@wsphillips wsphillips commented Aug 16, 2022

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
Copy link
Owner Author

unit tests and doc examples need updating

@wsphillips
Copy link
Owner Author

wsphillips commented Aug 22, 2022

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
Copy link
Owner Author

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

@wsphillips wsphillips merged commit 785b5bf into main Sep 2, 2022
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.

Start using new unit validation features in MTK
1 participant