fix: three code-review issues in analysis/satp-circe-notebook#131
Merged
MitchellAcoustics merged 2 commits intoanalysis/satp-circe-notebookfrom Mar 1, 2026
Merged
Conversation
…mat, error message Co-authored-by: MitchellAcoustics <22335636+MitchellAcoustics@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Conduct code review for analysis/satp-circe-notebook
fix: three code-review issues in analysis/satp-circe-notebook
Mar 1, 2026
MitchellAcoustics
merged commit d594993
into
analysis/satp-circe-notebook
2 checks passed
MitchellAcoustics
pushed a commit
that referenced
this pull request
Mar 1, 2026
#1 — RRuntimeError added to fit_exceptions rpy2's RRuntimeError inherits from Exception, not RuntimeError, so R-level convergence failures were escaping the per-model except block and crashing the entire fit_circe() call. Added import and included it in the fit_exceptions tuple. #2 — Guard p-value key access in extract_bfgs_fit Direct py_res["chisq"] / py_res["d"] bracket access replaced with .get() + None guard so a missing or None key raises a clear diagnostic rather than a bare KeyError or scipy TypeError. #4 — dtype preservation for d and m in mixed error/success DataFrames numpy int64 cannot hold NaN, so any error row with "d": None promoted the entire column to float64. Fixed by casting n, d, m to pd.Int64Dtype() (pandas nullable integer) after building the DataFrame. Extended test_fit_circe_error_row_preserves_numeric_dtypes to assert integer dtype for all three columns (n, d, m), not just n. Also: pandera participant field comment (| None vs nullable=True semantics) and models=[] edge case noted in fit_circe docstring. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.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.
Code review of the
analysis/satp-circe-notebookbranch introduces the SATP CircE analysis module, R session management, and SN wrappers. Three issues were identified and fixed.Changes
Bug:
install_r_packages()callsinstall.packages(character(0))on empty listAfter installing CircE from GitHub and removing it from
packnames_to_install,utils.install_packages(StrVector([]))was unconditionally called — passing an empty character vector to R'sinstall.packages. Added aif packnames_to_install:guard:Docstring format:
_circe_wrapper.pyused non-standard hybrid styleextract_bfgs_fit()andbfgs()used indented(type):parameter descriptions rather than the NumPy-style used everywhere else. Standardised to NumPy format.Error message:
fit_circe()empty-data message unconditionally mentionedparticipantThe
ValueErrorraised for an empty DataFrame said "Check that data contains valid rows with PAQ1-PAQ8 and participant column" even whenipsatize_data=False(where no participant column is needed). Message updated to remove the unconditional participant reference.🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.