-
Notifications
You must be signed in to change notification settings - Fork 61
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
Diffusion: tests, fixed coefficients in the solver, & tutorial #2226
Open
thorstenhater
wants to merge
90
commits into
arbor-sim:master
Choose a base branch
from
thorstenhater:bug/diffusion
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+957
−416
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…pointing to the fact that the diffusion across segments of different radius is still erroneous; see arbor-sim#2145)
…ternal-catalogues
…pointing to the fact that the diffusion across segments of different radius is still erroneous; see arbor-sim#2145)
…de readability improved
…ent_radii' again (for as long as the related fix is not merged)
@jlubo I finished testing on our GPU cluster and everything looks fine now. If you could help to repeat this on your cluster and clarify the scaling we can merge this and release 0.11 including the fixes. |
@jlubo's suggestions Co-authored-by: Jannik Luboeinski <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By careful re-analysis of the diffusion solver we found that the coeffiecients of the LHS need an additional factor of the CV Volume.
Scopes be creeping. During the analysis it became apparent that the during coupling to the cable equation proper raises at lot of potential problems.
Conversion of ionic current densities$i_X$ to mass transfer is $\dot c_X = \frac{i_XA}{q F V}$ with volume, area, Faraday's constant, and charge. Now, people are interested in using neutral species $n$ , ie $q=0$ for which we also should expect $i_n=0$ . Yet we need to special case on this (and assert zero current) to avoid ill-defined division. That's the smaller problem.
The more worrying issue is this:$\dot c_X = \frac{i_XA}{q F V}$ . Yet, there's no backreaction to the cable equation, unless the user explicitly constructs it. How? Well, the way ions should feed back to the voltage is this:
$\sim \ln\frac{X_i}{X_o}$
We construct a coupling term from the cable equation to the ionic diffusion via
iX = g(U-eX)
this is the conductance model and should be found in an NMODL file and
eX
which is the Nernst equation found, again, in a builtin NMODL file. But, for technical reasons we had to invent a special diffusive concentration
Xd
which is not identical toXi
. For proper behaviour, we should have usedXd
instead ofXi
in the Nernst term above. So, a non-standard Nernst module needs to be used.This brings me to the great change in this context: remove the offending term and carefully document how to retrofit it in NMODL and add the proper Nernst model, too.
fixes #2145
requires #2209