Skip to content

Fix magmap rank bug, magbin typo, plot.magbin speed; add tests and vignette sections#17

Draft
Copilot wants to merge 2 commits into
masterfrom
copilot/check-for-bugs-and-enhancements
Draft

Fix magmap rank bug, magbin typo, plot.magbin speed; add tests and vignette sections#17
Copilot wants to merge 2 commits into
masterfrom
copilot/check-for-bugs-and-enhancements

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 16, 2026

Thorough audit of the package: two silent bugs, several doc/typo issues, a dead-code remnant, a speed improvement in the hot drawing loop, and materially expanded test and vignette coverage.

Bugs fixed

magmap type='rank' produced no ranking (R/magmap.R)
data[good][order(data[good])] = locut:hicut creates an R temporary copy on the LHS — data was never modified, so every point mapped identically. Replaced with vectorised rank(..., ties.method='average').

# Before: all mapped values were identical (rank never applied)
magmap(c(3,1,4,1,5), type='rank')$map  # → same value repeated

# After: correctly spread across range
magmap(c(3,1,4,1,5), type='rank')$map  # → c(0.27, 0.00, 0.53, 0.00, 0.67) approx

magbin distance deduplication was a no-op (R/magbin.R line 226)
output$output$nn.distsoutput$output is NULL, so the assignment silently did nothing. Fixed to output$nn.dists.

Speed: plot.magbin

The per-bin loop checked x$shape on every iteration (constant across all bins) and tested !is.na(colmap$map[i]) in a conditional rather than pre-filtering. Refactored to select the drawing function once outside the loop and iterate only over which(!is.na(colmap$map)). Removed the now-redundant inner count > dustlim guard (bins are already filtered before the loop).

Dead code removed

magband.R: unused .Last.magroj local object (a stale copy of .Last.magproj) removed.

Documentation fixes

File Fix
man/magclip.Rd Typos: autoamtic, sepcific, cliped, Locial, "returns" → "returned"
man/magrun.Rd Return-value names bincen/binlimbincens/binlims (match code)
man/magmap.Rd "sue the output" → "use the output"

Tests added (tests/testthat/test-functions.R)

~50 new tests covering previously untested functions: magclip, maghist, magrun (including the corrected return names), magerr, magcurve, magcon, magbar, magcutout, and a regression test for the magmap rank bug.

Vignette additions

New worked-example sections in vignettes/magicaxis.Rmd: magclip, magrun, magerr, magcurve, magbar.

Copilot AI and others added 2 commits May 16, 2026 14:49
…dead code, add tests and vignette sections

Agent-Logs-Url: https://github.com/asgr/magicaxis/sessions/74e9c01b-f592-4d54-8e81-353d57d8b91c

Co-authored-by: asgr <5617132+asgr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants