Enhance GUI and benchmarking tools for COBA performance analysis;#105
Closed
Enhance GUI and benchmarking tools for COBA performance analysis;#105
Conversation
The float-to-bool conversion of the mv operator will proceed after thorough testing
- Implemented `COBA_2005_binary_fcnmv_boundary_CsvOuput.py` for benchmarking post and pre-synaptic connection updates using JAX. - Created `boundary_dis.py` for a GUI application to visualize performance boundaries and speedup analysis with interactive features. - Developed `dev_COBA_binary_fcnmv.py` for benchmarking with various connection probabilities and numbers, enhancing simulation capabilities. - Added error handling and CSV recording functionalities to capture benchmarking results effectively.
Contributor
Reviewer's GuideIntroduces warp-per-row CUDA scatter kernels with an auto-tuned TPR/WPR dispatcher for binary FCN matrix-vector ops, extends benchmarking/CSV tooling and adds new GUIs and scripts to explore COBA performance boundaries by backend, scale, connectivity, and VRAM limits. Class diagram for CSV_record and BenchmarkTools enhancementsclassDiagram
class CSV_record {
+str name
+str suffix
+str backend
+str operator
+str kernel
+str mode
+str dtype
+Path output_dir
+bool append
+int width
+list~dict~ rows
+list~str~ fieldnames
+dict _tags
+__init__(CSV_name, kernel, mode, duration, conn, suffix, output_dir, append)
+_write_csv(file_name, rows, fieldnames, mode)
+add_tag(tag_name, tag_value)
+add_row(row)
+single_COBA_data_add(operator, data_type, backend, mode, conn_num, scale, elapsed, rate, duration, homo)
+print_header(operator, data_type, backend, mode, conn_num, duration, homo, prob)
+print_table_header(show_conn)
+print_row(scale, size, elapsed, rate, conn_num)
+record_finish(tag)
}
class BenchmarkToolsModule {
+generate_params(dis_type, _N, limit_gb, target_samples, data_size, scale_max, conn_max) list
+memory_limit(conn_nums, scale, _N, limit, data_type) bool
+dump_jax_ir(func, args, kwargs, prefix)
}
class COBA_Benchmark_Script {
+benchmark_post_conn(conn_num, conn_prob, data_type, duration, homo, backend, probs_or_conn, _N)
+benchmark_pre_conn(conn_num, conn_prob, data_type, duration, homo, backend, probs_or_conn, _N)
}
CSV_record <.. BenchmarkToolsModule : uses
COBA_Benchmark_Script ..> CSV_record : creates
COBA_Benchmark_Script ..> BenchmarkToolsModule : calls
Class diagram for PerformanceBoundaryApp GUIclassDiagram
class PerformanceBoundaryApp {
+tk.Tk root
+Optional~pd.DataFrame~ df
+dict comboboxes
+dict tabs
+ttk.Combobox combo_compare_field
+ttk.Combobox combo_target
+ttk.Combobox combo_baseline
+ttk.Entry entry_n
+ttk.Entry entry_limit
+ttk.Entry entry_contours
+ttk.Entry entry_custom_lines
+ttk.Entry entry_dpi
+ttk.Entry entry_yellow_depth
+ttk.Entry entry_blue_depth
+ttk.Notebook notebook
+ttk.Frame filter_row
+__init__(root)
+load_data()
+update_plots()
+export_image()
+_setup_ui()
+_on_compare_field_changed(event)
+_rebuild_filter_row()
+_subtitle(include_baseline) str
+_render_scatter(tab, x, y, z, _N, limit_gb, x_min, x_max, y_max, z_label, cmap_name, subtitle)
+_render_interpolation(tab, x, y, z, _N, limit_gb, x_min, x_max, y_max, z_label, cmap_name, subtitle)
+_render_speedup_interp(tab, grid_x, grid_y, grid_z_masked, _N, limit_gb, x_min, x_max, y_max, subtitle)
+_draw_boundaries(ax, _N, limit_gb, x_min, x_max)
+_draw_contours_and_labels(ax, grid_x, grid_y, grid_z_masked, z_pts)
+_draw_custom_contours(ax, grid_x, grid_y, grid_z_masked)
+_setup_scatter_hover(tab, x, y, z, z_label)
+_setup_interp_hover(tab, grid_x, grid_y, grid_z, z_label)
}
class CSV_record {
+add_tag(tag_name, tag_value)
+add_row(row)
+record_finish(tag)
}
PerformanceBoundaryApp ..> CSV_record : reads CSV output
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
dev_COBA_binary_fcnmv.py’sbenchmark_post_connprobabilistic branch,single_COBA_data_addis called withcneven though onlyactual_conn_numis defined there, which will raise aNameErroror at least record incorrect connectivity metadata; this should useactual_conn_numinstead. - In
BenchmarkTools.CSV_record._write_csv,csv.DictWriteris now created withrestval='default', which will literally write the string "default" into any missing field; if this is not intentional, consider usingNoneor an empty string so that absent values remain visually distinguishable from a real string value. - In
_binary_fcnmv_cuda_kernel(binary.py), the kernel name for both scatter and gather now always uses the_boolspike suffix and you explicitly castspikestobool, makingspike_sfxeffectively unused; if this behavioral change (dropping non-bool spike paths) is intended, consider simplifying the signature and comments, otherwise restore the previous selection logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `dev_COBA_binary_fcnmv.py`’s `benchmark_post_conn` probabilistic branch, `single_COBA_data_add` is called with `cn` even though only `actual_conn_num` is defined there, which will raise a `NameError` or at least record incorrect connectivity metadata; this should use `actual_conn_num` instead.
- In `BenchmarkTools.CSV_record._write_csv`, `csv.DictWriter` is now created with `restval='default'`, which will literally write the string "default" into any missing field; if this is not intentional, consider using `None` or an empty string so that absent values remain visually distinguishable from a real string value.
- In `_binary_fcnmv_cuda_kernel` (binary.py), the kernel name for both scatter and gather now always uses the `_bool` spike suffix and you explicitly cast `spikes` to `bool`, making `spike_sfx` effectively unused; if this behavioral change (dropping non-bool spike paths) is intended, consider simplifying the signature and comments, otherwise restore the previous selection logic.
## Individual Comments
### Comment 1
<location path="dev/fcn/dev_COBA_binary_fcnmv.py" line_range="117-103" />
<code_context>
for s in scales:
actual_conn_num = int(s * prob * _N)
if actual_conn_num < 1 : actual_conn_num = 1
- if memory_limit(actual_conn_num, scale=s): continue
+ if BT.memory_limit(actual_conn_num, scale=s): continue
</code_context>
<issue_to_address>
**issue (bug_risk):** Prob-based post benchmark uses an undefined `cn` variable instead of `actual_conn_num` when recording results.
In the `probs_or_conn != 'conn'` branch of `benchmark_post_conn`, `cn` is undefined; this will either raise a `NameError` or cause incorrect metadata to be logged. Use `actual_conn_num` as the connection count argument instead, e.g.:
```python
csv_recorder.single_COBA_data_add(
'fcnmv', data_type, back, 'post', actual_conn_num, s,
elapsed, float(rate), duration,
homo=('homo' if homo else 'hetero'),
)
```
</issue_to_address>
### Comment 2
<location path="brainevent/_fcn/binary.py" line_range="275-284" />
<code_context>
- kernel_name = f'fcn_binary_mv.binary_fcnmv_scatter{mode_sfx}{spike_sfx}{sfx}'
+ #kernel_name = f'fcn_binary_mv.binary_fcnmv_scatter{mode_sfx}{spike_sfx}{sfx}'
+ kernel_name = f'fcn_binary_mv.binary_fcnmv_scatter{mode_sfx}_bool{sfx}'
else:
# Gather mode: y[i] = sum_k weights[i,k] * is_active(spikes[indices[i,k]])
# Auto-dispatch inside CUDA: TPR for n_conn<=512, MR for n_conn>512.
</code_context>
<issue_to_address>
**issue (bug_risk):** `spk_f` construction is inconsistent and appears unused/incorrectly overwritten.
In the non-`transpose` branch, `spk_f` is first built as a float mask and then immediately replaced by `u.math.asarray(spikes, dtype=bool)`, making the earlier construction dead code. Also, in the shown snippet `spk_f` is never used, so the conversion appears to have no effect.
Please either:
- remove the initial `spk_f` construction and keep only the representation actually needed, or
- ensure `spk_f` is the value converted and passed into the kernel if that’s what the kernel expects.
Aligning this with the kernel’s expected type will avoid subtle type/semantics issues and confusion from unused variables.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| t1 = time.time() | ||
| elapsed = t1 - t0 | ||
| csv_recorder.print_row(s, n, elapsed, float(rate)) | ||
| csv_recorder.single_COBA_data_add('fcnmv', data_type, back, 'post', cn, s, elapsed, float(rate), duration, homo=('homo' if homo else 'hetero')) |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Prob-based post benchmark uses an undefined cn variable instead of actual_conn_num when recording results.
In the probs_or_conn != 'conn' branch of benchmark_post_conn, cn is undefined; this will either raise a NameError or cause incorrect metadata to be logged. Use actual_conn_num as the connection count argument instead, e.g.:
csv_recorder.single_COBA_data_add(
'fcnmv', data_type, back, 'post', actual_conn_num, s,
elapsed, float(rate), duration,
homo=('homo' if homo else 'hetero'),
)
Comment on lines
275
to
284
| else: | ||
| # Gather mode: y[i] = sum_k weights[i,k] * is_active(spikes[indices[i,k]]) | ||
| # Auto-dispatch inside CUDA: TPR for n_conn<=512, MR for n_conn>512. | ||
| kernel_name = f'fcn_binary_mv.binary_fcnmv_gather{mode_sfx}{spike_sfx}{sfx}' | ||
| kernel_name = f'fcn_binary_mv.binary_fcnmv_gather{mode_sfx}_bool{sfx}' | ||
|
|
||
| def kernel(weights, indices, spikes): | ||
| #spikes = u.math.asarray(spikes, dtype=bool) | ||
| spikes = u.math.asarray(spikes, dtype=bool) | ||
| return jax.ffi.ffi_call(kernel_name, out_info)(weights, indices, spikes) | ||
|
|
||
| return kernel |
Contributor
There was a problem hiding this comment.
issue (bug_risk): spk_f construction is inconsistent and appears unused/incorrectly overwritten.
In the non-transpose branch, spk_f is first built as a float mask and then immediately replaced by u.math.asarray(spikes, dtype=bool), making the earlier construction dead code. Also, in the shown snippet spk_f is never used, so the conversion appears to have no effect.
Please either:
- remove the initial
spk_fconstruction and keep only the representation actually needed, or - ensure
spk_fis the value converted and passed into the kernel if that’s what the kernel expects.
Aligning this with the kernel’s expected type will avoid subtle type/semantics issues and confusion from unused variables.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Description
Type of Change
Changes Made
Testing
Test Configuration:
Test steps:
1.
2.
3.
Checklist
Screenshots/Outputs (if applicable)
Additional Context
Breaking Changes
Reviewer Notes:
Summary by Sourcery
Improve binary FCNMV CUDA scatter performance selection, strengthen COBA benchmarking utilities, and refine GUI-based performance analysis for backend-focused comparisons.
Enhancements: