Skip to content

fix: check resolved self.kernel_timings instead of stale profile param#168

Open
ItzDEXX wants to merge 1 commit into
brian-team:masterfrom
ItzDEXX:fix/profile-handling-bug-network-run
Open

fix: check resolved self.kernel_timings instead of stale profile param#168
ItzDEXX wants to merge 1 commit into
brian-team:masterfrom
ItzDEXX:fix/profile-handling-bug-network-run

Conversation

@ItzDEXX
Copy link
Copy Markdown

@ItzDEXX ItzDEXX commented Feb 20, 2026

Summary

Fixes #167

In GeNNDevice.network_run(), the deprecated preference fallback on line 1884 checked the original profile function parameter instead of the resolved self.kernel_timings value after reading from build_options. This caused set_device(..., profile=False) to be incorrectly overridden by prefs.devices.genn.kernel_timing = True.

Change

-            if profile is None and prefs.devices.genn.kernel_timing:
+            if self.kernel_timings is None and prefs.devices.genn.kernel_timing:


## Testing
Wrote 8 unit tests that simulate the profile resolution logic in isolation (no GeNN/brian2 required). Tests cover all precedence combinations: explicit `profile` arg, `build_options` values, and deprecated preference fallback. Includes a test that reproduces the exact bug scenario (`build_options=False` + `kernel_timing=True`) and verifies the old logic fails while the fixed logic correctly returns `False`. **All 8 tests passed.**

@ItzDEXX
Copy link
Copy Markdown
Author

ItzDEXX commented Feb 20, 2026

@mstimberg @kernfel Look into this

@mstimberg
Copy link
Copy Markdown
Member

Thank you for the PR @ItzDEXX . I won't get around reviewing it very soon, I'm afraid, since I am about to go on holiday. Please also note that brian2genn has not been seeing any development from our side for a while, since from Brian's side we are rather focused on https://github.com/brian-team/brian2cuda for GPU-accelerated simulations. GeNN itself is being actively developed, but the brian2genn interface hasn't kept up with the new features and functionalities it offers.

@mstimberg
Copy link
Copy Markdown
Member

Hi @ItzDEXX, I had a quick look at the PR, and it seems indeed to fix a bug, but does the code actually work correctly for a run(..., profile=True)? You wrote "Wrote 8 unit tests", but I don't see any tests in your PR.

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.

Bug: network_run profile handling checks stale parameter instead of resolved value

2 participants