refactor(r_wrapper): consolidate internal R wrapper into single module#140
Merged
Conversation
- Remove EmbeddedRPackage proxy (bypassed by its own module) - Remove _state.session and _state.circe fields; add circe_sourced: bool - Flatten check_dependencies() — remove redundant nested check_r_availability() calls inside check_sn_package() and check_circe_package() - Remove _confirm_install_r_packages() and all TTY/env-var auto-install logic; ImportError with clear instructions instead - initialize_r_session() returns None (no caller used the dict return) - Remove is_session_active() thin wrapper - Fix double-check in install_r_packages() - Extract _np2rmat() in _rsn_wrapper, replacing 3x duplicated R matrix boilerplate - Batch-draw in sample_mtsn (max(n,64) per R call) instead of n=1 per candidate - Update tests to assert on _state.active / _state.circe_sourced directly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove remaining scaffolding and collapse the CircE two-step API: - Drop importlib.metadata and NoReturn imports (both now dead) - Remove _state.stats — loaded but never accessed by any wrapper - Remove is_ready property — active=True is set atomically after all packages load, so the check in get_r_session() can never trigger - Inline the _raise_*_error inner closures in all three check functions; raises directly in except blocks, removing ~20 lines of define-then-call scaffolding with no diagnostic benefit - Simplify check_circe_package(): drop redundant _get_circe_embedded_paths() call and outer try/except — _source_embedded_circe_scripts() already converts every exception to ImportError internally - Merge bfgs() + extract_bfgs_fit() into bfgs_fit() returning dict[str, Any]; the ro.ListVector intermediate never left the call chain and is now gone - Update circe.py: compute_bfgs_fit() calls bfgs_fit(), from_bfgs() accepts fit_stats: dict instead of a raw R ListVector - Update tests: TestBfgsWrapper uses bfgs_fit() directly; drop the test that asserted on the internal ro.ListVector type Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Move CircE and skew-normal wrapper functions into `_r_wrapper`, remove the old split modules, and update imports and tests to use the unified internal API.
Removed separate CircE wrapper and skew-normal wrapper sections — all three are now documented from the single _r_wrapper module. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
MitchellAcoustics
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the optional R integration layer by consolidating previously split internal wrappers into a single soundscapy.r_wrapper._r_wrapper module, simplifying session state handling and updating SATP/SPI integration points accordingly.
Changes:
- Consolidated CircE and
snwrapper functionality intosrc/soundscapy/r_wrapper/_r_wrapper.pyand removed the now-redundant_circe_wrapper.py/_rsn_wrapper.pymodules. - Simplified R session state to a single
RSessionwith lazy initialization and updated wrappers/tests to use the new session model. - Updated SATP CircE fitting to call a new
bfgs_fit()helper that returns extracted fit statistics directly, plus updated internal docs and version metadata.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/spi/test_r_wrapper.py | Updates session-management tests to the new lazy get_r_session() / _state model. |
| test/satp/test_circe.py | Migrates CircE numerical regression tests to use bfgs_fit() returning a dict. |
| src/soundscapy/satp/circe.py | Switches SATP fitting pipeline to consume extracted fit stats (dict) rather than raw R model objects. |
| src/soundscapy/r_wrapper/_r_wrapper.py | Consolidates all R wrapper logic (session init/reset, sn helpers, CircE BFGS fit extraction) into a single module. |
| src/soundscapy/r_wrapper/init.py | Updates re-exports to match the consolidated wrapper API (bfgs_fit, etc.). |
| src/soundscapy/r_wrapper/_rsn_wrapper.py | Deleted (functionality moved into _r_wrapper.py). |
| src/soundscapy/r_wrapper/_circe_wrapper.py | Deleted (functionality moved into _r_wrapper.py). |
| docs/reference/internals/r-internals.md | Updates internal documentation to reference the consolidated module. |
| pyproject.toml | Bumps package version to 0.8.4.dev1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/MitchellAcoustics/Soundscapy/sessions/d8340c82-4312-4551-8333-b0328a7a0c9d Co-authored-by: MitchellAcoustics <22335636+MitchellAcoustics@users.noreply.github.com>
…tVector input Agent-Logs-Url: https://github.com/MitchellAcoustics/Soundscapy/sessions/9e7d4c44-7307-4f40-b931-578e0ab982a4 Co-authored-by: MitchellAcoustics <22335636+MitchellAcoustics@users.noreply.github.com>
…from_bfgs() Agent-Logs-Url: https://github.com/MitchellAcoustics/Soundscapy/sessions/9e7d4c44-7307-4f40-b931-578e0ab982a4 Co-authored-by: MitchellAcoustics <22335636+MitchellAcoustics@users.noreply.github.com>
…t_bfgs_stats() Agent-Logs-Url: https://github.com/MitchellAcoustics/Soundscapy/sessions/9e7d4c44-7307-4f40-b931-578e0ab982a4 Co-authored-by: MitchellAcoustics <22335636+MitchellAcoustics@users.noreply.github.com>
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
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.
Summary
The
r_wrappermodule was split across three private files (_r_wrapper.py,_rsn_wrapper.py,_circe_wrapper.py) with ~500 lines, 8 public functions, and a single R session — more structure than the scope warranted. This PR collapses it to one file and removes redundant state.Changes:
_rsn_wrapper.pyand_circe_wrapper.pyinto_r_wrapper.py— one file to read when debugging R issues, no cross-file importsRSessionfrom 7 fields to 3 (sn,base,active) — the four check flags (r_checked,sn_checked,circe_checked,circe_sourced) are all implied byactive=True_initialize_r_session()functioninstall_r_packages()as aDeprecationWarning-only shim — it was dead code (never called outside the module) and implied the library manages R package installationdocs/reference/internals/r-internals.mdto document the single consolidated module (fixes docs build failure)No public API changes. All 103 tests pass across all platforms.
Test plan
pixi run pytest test/spi/test_r_wrapper.py— session management and_vertestspixi run pytest test/satp/test_circe.py— CircE BFGS numerical regression anchors (chisq, rmsea, cfi, etc.)pixi run pytest test/spi/— full MSN fitting/sampling suite🤖 Generated with Claude Code