Skip to content

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

Merged
asgr merged 3 commits into
masterfrom
copilot/check-for-bugs-and-enhancements
May 18, 2026
Merged

Fix magmap rank bug, magbin typo, plot.magbin speed; add tests and vignette sections#17
asgr merged 3 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>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR is a maintenance audit of the magicaxis package that fixes two silent bugs, removes dead code, and speeds up the inner drawing loop of plot.magbin. It also corrects several documentation typos and adds a substantial new test file plus vignette sections covering previously undocumented functions.

Changes:

  • Bug fixes: magmap(type='rank') now actually ranks values via rank(..., ties.method='average'); magbin now correctly assigns to output$nn.dists (the previous output$output$nn.dists was a silent no-op on NULL).
  • Performance/cleanup: plot.magbin hoists the shape branch out of the per-bin loop and iterates only over non-NA colmap$map entries; dead .Last.magroj block removed from magband.R.
  • Docs & tests: typo fixes in magclip.Rd, magmap.Rd, magrun.Rd (incl. bincen/binlimbincens/binlims); new tests/testthat/test-functions.R (~50 tests) and new vignette sections for magclip, magrun, magerr, magcurve, magbar.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
R/magmap.R Replace ineffective data[good][order(...)] = locut:hicut with rank().
R/magbin.R Fix output$output$nn.dists no-op; refactor plot.magbin draw loop for clarity/speed.
R/magband.R Remove unused .Last.magroj local object.
man/magclip.Rd Spelling and grammar corrections.
man/magmap.Rd Typo: "sue" → "use".
man/magrun.Rd Rename return items bincen/binlim to match code (bincens/binlims).
tests/testthat/test-functions.R New unit tests for several previously untested functions and the rank bug regression.
vignettes/magicaxis.Rmd New worked-example sections for magclip, magrun, magerr, magcurve, magbar.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@asgr asgr marked this pull request as ready for review May 18, 2026 02:19
…ner alias for magtri too (since lots of people call them corner plots).
@asgr asgr merged commit a5e2ce3 into master May 18, 2026
3 checks passed
@asgr asgr deleted the copilot/check-for-bugs-and-enhancements branch May 18, 2026 08:14
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.

3 participants