Skip to content

Fix RxD MultiCompartment Reactions with different number of source and destination terms.#3805

Open
adamjhn wants to merge 4 commits into
masterfrom
rxd_mcr_mult_fix
Open

Fix RxD MultiCompartment Reactions with different number of source and destination terms.#3805
adamjhn wants to merge 4 commits into
masterfrom
rxd_mcr_mult_fix

Conversation

@adamjhn

@adamjhn adamjhn commented Jun 23, 2026

Copy link
Copy Markdown
Member

No description provided.

adam added 2 commits June 19, 2026 21:34
… were not copied correctly when the number of sources and destinations were not equal.
@adamjhn adamjhn requested a review from ramcdougal June 23, 2026 16:21
@adamjhn adamjhn added bug rxd reaction-diffusion labels Jun 23, 2026
@azure-pipelines

Copy link
Copy Markdown

✔️ cb5ec30 -> Azure artifacts URL

@codecov

codecov Bot commented Jun 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 68.43%. Comparing base (2ac5cc7) to head (cb5ec30).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3805      +/-   ##
==========================================
- Coverage   68.45%   68.43%   -0.03%     
==========================================
  Files         688      688              
  Lines      111300   111300              
==========================================
- Hits        76187    76163      -24     
- Misses      35113    35137      +24     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

/ molecules_per_mM_um3
for s, si in zip(dests_ecs, sources_indices)
for s in dests_ecs
for si in sources_indices

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can be right. The line that uses s and si seems like they should go together in pairs, but this has us doing all possible pairwise combinations. At a minimum, this changes the behavior when the lists are the same size (e.g., if both were of length 2, before we'd have two entries in the list comprehension; this would give 4.


h, rxd, data, save_path = neuron_instance
dend = h.Section(name="dend")
cyt = rxd.Region(h.allsec(), name="cyt", nrn_region="i")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, I'm opposed to allsec on principle. Why not just call it [dend]?

multiplier array for other reactions."""

h, rxd, data, save_path = neuron_instance
dend = h.Section(name="dend")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name= is unnecessary.

/ molecules_per_mM_um3
for s, di in zip(sources_ecs, dests_indices)
for s in sources_ecs
for di in dests_indices

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can be right. The line that uses s and di seems like they should go together in pairs, but this has us doing all possible pairwise combinations. At a minimum, this changes the behavior when the lists are the same size (e.g., if both were of length 2, before we'd have two entries in the list comprehension; this would give 4.

@sonarqubecloud

Copy link
Copy Markdown

@azure-pipelines

Copy link
Copy Markdown

✔️ 59b8666 -> Azure artifacts URL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug rxd reaction-diffusion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants