Tests for particle diffusion #2209
Closed
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
|
Hey @jlubo, thanks for adding this. Could you run |
|
And |
|
Okay, |
thorstenhater
requested changes
Aug 16, 2023
…pointing to the fact that the diffusion across segments of different radius is still erroneous; see arbor-sim#2145)
jlubo
force-pushed
the
tests_for_particle_diffusion
branch
from
74a8f99 to
2c1e802
Compare
Thanks @thorstenhater! I've revised the code according to the comments. |
thorstenhater
requested changes
Aug 25, 2023
jlubo
force-pushed
the
tests_for_particle_diffusion
branch
from
25fe938 to
7f77d60
Compare
thorstenhater
previously approved these changes
Aug 25, 2023
|
cscs-ci run daint-gpu |
|
Uh-oh @boeschf could you take a look? I suspect the recent changes to event delivery clash with the diffusion handling: if (ii_ < (end_ - begin_)) {
const auto tid_ = (begin_ + ii_)->mech_index;
if ((ii_ > 0) && ((begin_ + (ii_ - 1))->mech_index == tid_)) return;
for (auto i_ = begin_ + ii_; i_ < end_; ++i_) {
if (i_->mech_index != tid_) break;
[[maybe_unused]] auto weight = i_->weight;
unsigned lane_mask_ = arb::gpu::ballot(0xffffffff, tid_<n_);this was emitted by modcc for the NET_RECV. Note the missing |
|
The different-radii and different-length tests are now uncommented. They should succeed if they are run with this new fix in the diffusion solver: thorstenhater@7577092 |
|
Hey @jlubo, could you leave them disabled, I'll add them to the to-be-made PR for the fixes. |
…ent_radii' again (for as long as the related fix is not merged)
|
Done :) |
|
I'll fix the GPU problem in my follow-up #2226 |
thorstenhater
requested changes
Oct 10, 2023
There was a problem hiding this comment.
Nice! See also downstream #2226. Two questions:
- It seems like lots of special casing for the different setups (num_sgs=1,2,3). Do you think it would be cleaner to split these into different tests altogether?
- Question also inline: Can't we always compute the equilibrium concentration?
I agree, I've now split the function
Yes we can! I've introduced a more general computation of CV volumes making this possible now. |
|
cscs-ci run daint-gpu |
This was referenced Jan 9, 2024
|
Yes, sure. I'll check #2226 again in the next days. |
thorstenhater
added a commit
that referenced
this pull request
Jan 22, 2025
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:
We construct a coupling term from the cable equation to the ionic
diffusion via $\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:
`iX = g(U-eX)`
this is the conductance model and should be found in an NMODL file and
`eX` $\sim \ln\frac{X_i}{X_o}$
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 to `Xi`. For proper behaviour, we should
have used `Xd` instead of `Xi` 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
---------
Co-authored-by: Jannik Luboeinski <[email protected]>
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.
Python tests are added to ensure the working of the diffusion of arbitrary particles in setups with 1-3 segments (cf. issues #2084 and #2145).
Besides showing that the diffusion seems to work if segments have the same radius, the tests that are added here also point to the fact that the diffusion across segments of different radius is still erroneous. Therefore, I left the tests for different radii commented until #2145 has been resolved.