Skip to content

146 add support for multi element channel modeling#205

Open
capn-freako wants to merge 16 commits into
masterfrom
146-add-support-for-multi-element-channel-modeling
Open

146 add support for multi element channel modeling#205
capn-freako wants to merge 16 commits into
masterfrom
146-add-support-for-multi-element-channel-modeling

Conversation

@capn-freako

@capn-freako capn-freako commented Jun 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

I turned Claude loose on this issue and I think we've got a workable first-pass solution, after some manual "adjustments" to his fix.

Still to do:

  • Make sure we're actually testing correct s4p concatenation by PyBERT.

capn-freako and others added 7 commits June 16, 2026 08:27
@capn-freako @claude
Add composite interconnect support and fix IBIS-AMI getwave test
bc19c48 
- Expand ch_file trait to ch_files list; add Add/Remove/Clear buttons
  and a read-only queue display so users can build a composite channel
  by cascading multiple S-parameter/time-domain files via skrf **
- Persist ch_files in PyBertCfg (backward-compat: falls back to ch_file
  when list is empty)
- Fix IBISModel() calls: drop stale third positional arg that no longer
  exists in the updated pyibisami signature
- Fix run_ami_model(): use AmiModelResponseKey constants (not plain
  strings) when indexing the dict returned by AMIModel.get_responses()
- Add CLAUDE.md with commands, architecture, and key invariants

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@capn-freako @claude
Update uv.lock
f56b000 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@capn-freako @claude
Update PyChOpMarg submodule
7d3c66c 
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@capn-freako
Fixed a bug in the path name formatter.
2b78a07
@capn-freako @claude
Absorb channel file browsing into Add button
c130a8e 
The separate FileEditor browse widget is removed; clicking Add now opens
a native pyface FileDialog directly and appends the chosen path to
ch_files in one step.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@capn-freako
GUI rearrangement.
b60a4ea
@capn-freako
Some manual adjustments to Claude's solution.
fc5c947
@capn-freako capn-freako linked an issue Jun 17, 2026 that may be closed by this pull request
Add support for multi-element channel modeling. #146
Open
@capn-freako capn-freako requested a review from jdpatt June 17, 2026 11:26
@jdpatt

jdpatt commented Jun 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Didn't get much time to dig deep but a few changes/comments off the cuff. I don't know if you ever tried my branch where I was removing traits and also added this feature and had sent for you to demo. Deltas from that and features worth while adding:

  • Should use a radio button group to switch widgets between native/files. Much nicer user visual of which is enabled vs the gray out or not. Really this might be nice for all builtin or external features.
  • Add/Remove Last/Clear All could get annoying. I had and propose to add here a Up/Down Arrow or drag handle per Row to reorder rows + a "X" at the end to remove that row. You could have a single Add or Remove all at top then.
  • I think we could drop "Section" from the buttons as that is apparent.
  • I assume fix ports and applying a window or eventual manual port reordering should be per file not applied to all files. Which would ripple into the user configuration object.
  • Let claude add some test coverage of a single file, multiple files and etc.. Use scikit-rf to generate said networks; make sure the fix ports is applying to all/some or a mix.

@capn-freako

capn-freako commented Jun 19, 2026

Copy link
Copy Markdown
Owner Author
  • Should use a radio button group to switch widgets between native/files. Much nicer user visual of which is enabled vs the gray out or not. Really this might be nice for all builtin or external features.

Sorry, do you mean that the user's radio button selection makes the irrelevant half disappear?
(That's how I'm reading, "much nicer than graying it out.")

@capn-freako

capn-freako commented Jun 19, 2026

Copy link
Copy Markdown
Owner Author
  • Add/Remove Last/Clear All could get annoying. I had and propose to add here a Up/Down Arrow or drag handle per Row to reorder rows + a "X" at the end to remove that row. You could have a single Add or Remove all at top then.

Yes! That'd be much nicer.
Do you have time to make this change or shall I sick Claude on it?

@capn-freako

capn-freako commented Jun 19, 2026

Copy link
Copy Markdown
Owner Author
  • I think we could drop "Section" from the buttons as that is apparent.

Agreed; unnecessarily verbose.
I'll remove it.

@capn-freako

capn-freako commented Jun 19, 2026

Copy link
Copy Markdown
Owner Author
  • I assume fix ports and applying a window or eventual manual port reordering should be per file not applied to all files. Which would ripple into the user configuration object.

I think I disagree here.
Here's my reasoning:

  • Fix ports: This merely gives PyBERT the green light to assess the port ordering convention used, changing it only when it finds that the "1->3/2->4" convention was used.
    So, some sections (those having used the "1->3/2->4" convention) would be corrected, while others (those using the "1->2/3->4" convention) would not.

  • Use window: When this option is enabled by the user, the windowing is only applied to the final composite network, not each sub-section individually.

Let's discuss this one some more.

@capn-freako

capn-freako commented Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner Author
  • Let claude add some test coverage of a single file, multiple files and etc.. Use scikit-rf to generate said networks; make sure the fix ports is applying to all/some or a mix.

Great suggestion!
(And an interesting test of Claude's abilities. ;-) )
I'll poke at that, today.

Okay, checkout 9d629e4 and see if you think the additional testing is sufficient.

capn-freako and others added 4 commits June 19, 2026 07:54
@capn-freako
Removed "Section" from interconnect building buttons, as per David Pa…
dedef32 
…tterson's suggestion.
@capn-freako
Add tests for multi-element composite channel building.
b7effd0 
Uses scikit-rf to construct reference transmission-line networks and verifies
that cascading N s2p files via import_channel matches direct skrf cascades,
including 2- and 3-segment unit tests and an end-to-end PyBERT simulation test.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Prompt used:
> Test the new composite channel building functionality. Use scikit-rf to build the reference channel for testing.
@capn-freako @claude
Expand composite channel tests to cover both s4p port numbering conve…
7bfaf15 
…ntions.

Adds TestPortRenumber with 6 tests verifying that renumber=True correctly
normalises both the standard "1→2" and alternative "1→3" 4-port conventions,
including negative tests (renumber=False on a 1→3 file gives Sdd21≈0) and
mixed-convention cascade scenarios.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Prompt used:
> Expand testing to ensure that port reordering (i.e. - renumber = True) works with a mix of both s4p port numbering conventions.
@capn-freako
Updated CLAUDE.md.
9d629e4
@jdpatt

jdpatt commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator
  • Should use a radio button group to switch widgets between native/files. Much nicer user visual of which is enabled vs the gray out or not. Really this might be nice for all builtin or external features.

Sorry, do you mean that the user's radio button selection makes the irrelevant half disappear?

(That's how I'm reading, "much nicer than graying it out.")

We can spilt this off in another task but exactly. It's not which it's using; you only see the one you selected.

@jdpatt

jdpatt commented Jun 20, 2026

Copy link
Copy Markdown
Collaborator

Ack on the reordering and tests! I won't be off mobile only until Monday; so might as well let Claude augment with the drag handle and remove per row. I'll try it out and review in more detail then.

capn-freako and others added 3 commits June 20, 2026 09:59
@capn-freako
GUI rearranging, as per David Patterson's suggestions.
97a71ea
@capn-freako @claude
Fix configuration save/load after Bool→Enum trait rename.
20ce018 
The GUI rearrangement commit replaced three Bool traits with Enum
equivalents (use_ch_file→inter_sel, tx_use_ibis→tx_sel,
rx_use_ibis→rx_sel) without updating PyBertCfg, breaking save_configuration
and the CLI sim test. Also adds backward-compat mapping in load_from_file
so old saved configs still load correctly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@capn-freako @claude
Fix test_empty_ch_files_raises after Bool→Enum trait rename.
f97491f 
Updates the test to use `inter_sel = "multiple"` instead of the
removed `use_ch_file = True` Bool trait, so the RuntimeError path
is actually exercised.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@capn-freako

capn-freako commented Jun 20, 2026

Copy link
Copy Markdown
Owner Author
  • Add/Remove Last/Clear All could get annoying. I had and propose to add here a Up/Down Arrow or drag handle per Row to reorder rows + a "X" at the end to remove that row. You could have a single Add or Remove all at top then.

Yes! That'd be much nicer. Do you have time to make this change or shall I sick Claude on it?

@jdpatt , when you get a chance, take the latest commit for a test drive and see if you like the new format/behavior of the channel configuration GUI better than the old way.

@capn-freako

capn-freako commented Jun 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner Author
  • Should use a radio button group to switch widgets between native/files. Much nicer user visual of which is enabled vs the gray out or not. Really this might be nice for all builtin or external features.

Sorry, do you mean that the user's radio button selection makes the irrelevant half disappear?
(That's how I'm reading, "much nicer than graying it out.")

We can spilt this off in another task but exactly. It's not which it's using; you only see the one you selected.

Okay, I'll create a new Issue for this, as you suggest.

==> See Issue #206

@capn-freako @claude
Fix IBIS-AMI test fixtures and expand unit test coverage.
c290a59 
Three IBIS-AMI fixtures in conftest.py were still using the removed Bool
trait rx_use_ibis=True (silently ignored by HasTraits) instead of the
new rx_sel="ibis" Enum, causing 30 tests to pass while testing the native
path instead of IBIS. The test methods in all three IBIS test files were
also requesting the wrong fixture (dut instead of the IBIS fixture), so
they were never exercising the IBIS code path. Both problems are fixed;
a test_rx_sel guard is added to each class to catch this category of
regression. A leftover debug print in sparam.py is also removed.

New test files cover previously untested utility modules: test_math.py
(safe_log10, gaus_pdf, lfsr_bits, make_bathtub, all_combs), test_sigproc.py
(resize_zero_pad, moving_average, raised_cosine, trim_impulse), test_jitter.py
(find_crossing_times), test_channel.py (calc_gamma_RLGC), and test_fec.py
(FEC_Encoder/FEC_Decoder round-trip). A backward-compat config loading test
is added to test_loading_and_saving.py to cover the Bool→Enum mapping
introduced in configuration.py on this branch.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdpatt jdpatt Awaiting requested review from jdpatt

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add support for multi-element channel modeling.

2 participants

@capn-freako @jdpatt