Robustness fixes + review report#27
Conversation
|
@copilot resolve the merge conflicts in this pull request |
1 similar comment
|
@copilot resolve the merge conflicts in this pull request |
|
@copilot resolve the merge conflicts in this pull request |
|
@copilot resolve the merge conflicts in this pull request |
1 similar comment
|
@copilot resolve the merge conflicts in this pull request |
There was a problem hiding this comment.
Pull request overview
Adds a Quarto-based review report to the repository and applies a set of small robustness/maintainability fixes to the jjmR R package code (file existence checks, safer option handling, and removal of eval(parse())).
Changes:
- Added
reviews/artifacts: the review source (.qmd), rendered HTML, and a patch file capturing the proposed fixes. - Hardened model reading/execution paths (
readJJM(),.getOutFile(),.runJJM()) and improved error handling for missing outputs. - Simplified/safer internals: replaced
eval(parse())withdo.call(), added column-count validation inget_index_fits(), and relaxed strict class checks withinherits().
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| reviews/jjmr_review.qmd | Adds the review report source document describing issues and recommended fixes. |
| reviews/jjmr_review.html | Adds rendered HTML version of the report. |
| reviews/jjmr_fixes.patch | Adds a patch file containing the proposed code changes for reference. |
| R/readJJMConfig.R | Fixes readJJMConfig.jjm.output() by adding the missing output argument. |
| R/readJJM.R | Adds explicit output-file existence checks and warning for missing .rep files. |
| R/jjmr-internal.R | Makes .getOutFile() tolerant of missing files and fixes .runJJM() adflags handling. |
| R/jjm.output-internal.R | Makes .combineModels() class checks robust via inherits(). |
| R/get_index_fits.R | Adds validation to ensure expected index-fit column counts before renaming. |
| R/compareModels.R | Replaces eval(parse()) command assembly with do.call(). |
Comments suppressed due to low confidence (1)
R/readJJMConfig.R:44
- The new
outputargument inreadJJMConfig.jjm.output()is later shadowed byoutput = .getJjmConfig(...)in the same function (and similarly inreadJJMConfig.default). This makes the code harder to reason about and can lead to subtle bugs ifoutputis referenced after the assignment. Rename the return variable (e.g.,cfg/config) to avoid clobbering theoutputparameter.
readJJMConfig.jjm.output = function(model, path, input=NULL, output="results", ...) {
ctl = .getCtlFile(model=model, path=path) # path to ctl file
dat = .getDatFile(ctl=ctl, input=input) # path to dat file
reps = .getRepFiles(model=model, output=output)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| ## Scope | ||
|
|
||
| Review of the R package source in `jjmr/`, focusing on likely bugs, robustness, and maintainability issues. Notes below reference file names and functions. |
| ## Executive summary | ||
|
|
||
| Primary risks are hard failures during routine use: (1) `readJJMConfig.jjm.output()` errors immediately due to an undefined `output`; (2) `.runJJM()` can build an empty command when `adflags = NULL`; and (3) `readJJM()` can fail before reading outputs because `normalizePath()` expects existing files. Secondary issues affect robustness and reproducibility (assumptions about fixed column counts and shared fleet metadata), plus maintainability items (use of `eval(parse())`, strict class checks). Fixes are small, localized, and low‑risk. | ||
|
|
||
| ## Summary (high‑priority items) | ||
|
|
||
| - **Confirmed runtime error** in `readJJMConfig.jjm.output()` due to an undefined `output` variable. | ||
| - **Likely runtime error** in `.runJJM()` when `adflags = NULL` (default): the current check uses `exists("adflags")`, which is always `TRUE` and can collapse the command to `character(0)`. | ||
| - **Potential failure** in `readJJM()`/`.getOutFile()` when output files are missing because `normalizePath()` defaults to `mustWork = TRUE`. |
| <script src="jjmr_review_files/libs/clipboard/clipboard.min.js"></script> | ||
| <script src="jjmr_review_files/libs/quarto-html/quarto.js" type="module"></script> | ||
| <script src="jjmr_review_files/libs/quarto-html/tabsets/tabsets.js" type="module"></script> | ||
| <script src="jjmr_review_files/libs/quarto-html/axe/axe-check.js" type="module"></script> | ||
| <script src="jjmr_review_files/libs/quarto-html/popper.min.js"></script> | ||
| <script src="jjmr_review_files/libs/quarto-html/tippy.umd.min.js"></script> | ||
| <script src="jjmr_review_files/libs/quarto-html/anchor.min.js"></script> | ||
| <link href="jjmr_review_files/libs/quarto-html/tippy.css" rel="stylesheet"> | ||
| <link href="jjmr_review_files/libs/quarto-html/quarto-syntax-highlighting-ed96de9b727972fe78a7b5d16c58bf87.css" rel="stylesheet" id="quarto-text-highlighting-styles"> | ||
| <script src="jjmr_review_files/libs/bootstrap/bootstrap.min.js"></script> | ||
| <link href="jjmr_review_files/libs/bootstrap/bootstrap-icons.css" rel="stylesheet"> | ||
| <link href="jjmr_review_files/libs/bootstrap/bootstrap-d6a003b94517c951b2d65075d42fb01b.min.css" rel="stylesheet" append-hash="true" id="quarto-bootstrap" data-mode="light"> |
| --- | ||
| title: "jjmR Code Review" | ||
| format: html | ||
| execute: | ||
| eval: false | ||
| --- |
No description provided.