arbor icon indicating copy to clipboard operation
arbor copied to clipboard

Tests for particle diffusion

Open jlubo opened this issue 1 year ago • 12 comments

Python tests are added to ensure the working of the diffusion of arbitrary particles in setups with 1-3 segments (cf. issues https://github.com/arbor-sim/arbor/issues/2084 and https://github.com/arbor-sim/arbor/issues/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 https://github.com/arbor-sim/arbor/issues/2145 has been resolved.

jlubo avatar Aug 15 '23 09:08 jlubo

Hey @jlubo,

thanks for adding this. Could you run black . and push the results? That'll make the Linter check pass.

thorstenhater avatar Aug 16 '23 09:08 thorstenhater

And flake8 . :D

thorstenhater avatar Aug 16 '23 12:08 thorstenhater

Okay, black now passes (I had previously used an older version, seems that they changed something). Yeah, now flake8 is complaining... I'll try to fix that, too :)

jlubo avatar Aug 16 '23 12:08 jlubo

Hey, made some comments for improvements, rest looks great, thanks.

Thanks @thorstenhater! I've revised the code according to the comments.

jlubo avatar Aug 24 '23 14:08 jlubo

cscs-ci run daint-gpu

thorstenhater avatar Aug 29 '23 07:08 thorstenhater

Uh-oh

subprocess.CalledProcessError: Command 'make' returned non-zero exit status 2.
 Error:
/tmp/tmpqgxm6fo0/build/generated/diffusion/neuron_with_diffusion_gpu.cu(59): warning #177-D: function "arb::diffusion_catalogue::<unnamed>::multiply" was declared but never referenced
/tmp/tmpqgxm6fo0/build/generated/diffusion/synapse_with_diffusion_gpu.cu(74): error: identifier "n_" is undefined
/tmp/tmpqgxm6fo0/build/generated/diffusion/synapse_with_diffusion_gpu.cu(55): warning #177-D: function "arb::diffusion_catalogue::<unnamed>::multiply" was declared but never referenced
1 error detected in the compilation of "/tmp/tmpqgxm6fo0/build/generated/diffusion/synapse_with_diffusion_gpu.cu".
make[2]: *** [CMakeFiles/diffusion-catalogue.dir/build.make:183: CMakeFiles/diffusion-catalogue.dir/generated/diffusion/synapse_with_diffusion_gpu.cu.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:83: CMakeFiles/diffusion-catalogue.dir/all] Error 2

@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 n_, which I assume should be end_ - begin_ here?

thorstenhater avatar Aug 29 '23 08:08 thorstenhater

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: https://github.com/thorstenhater/arbor/commit/7577092f89e9b54d8fde26ac6963a7c9965e43cc

jlubo avatar Oct 07 '23 19:10 jlubo

Hey @jlubo,

could you leave them disabled, I'll add them to the to-be-made PR for the fixes.

thorstenhater avatar Oct 09 '23 13:10 thorstenhater

Done :)

jlubo avatar Oct 10 '23 09:10 jlubo

I'll fix the GPU problem in my follow-up #2226

thorstenhater avatar Oct 10 '23 12:10 thorstenhater

1. 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?

I agree, I've now split the function get_morph_and_decor() into into individual functions for 1, 2, and 3 segments.

2. Question also inline: Can't we always compute the equilibrium concentration?

Yes we can! I've introduced a more general computation of CV volumes making this possible now.

jlubo avatar Oct 14 '23 15:10 jlubo

cscs-ci run daint-gpu

boeschf avatar Oct 23 '23 12:10 boeschf