Skip to content

Updates for compatibility with Huber-group-EMBL/romeo - #54

Merged
Artur-man merged 9 commits into
develfrom
romeo_class
Aug 10, 2026
Merged

Updates for compatibility with Huber-group-EMBL/romeo#54
Artur-man merged 9 commits into
develfrom
romeo_class

Conversation

@Artur-man

@Artur-man Artur-man commented Aug 8, 2026

Copy link
Copy Markdown
Collaborator
  • export ImageArray class
  • remove axes slot and parse axes ordering from scale
  • update scaling strategy from 1/s or s

@Artur-man

Copy link
Copy Markdown
Collaborator Author

@claude review

@claude

This comment was marked as outdated.

@Artur-man

Copy link
Copy Markdown
Collaborator Author

@claude review again

@claude

This comment was marked as outdated.

@Artur-man

Copy link
Copy Markdown
Collaborator Author

@claude review, check again but make it short

@claude

claude Bot commented Aug 8, 2026

Copy link
Copy Markdown

Claude finished @Artur-man's task in 2m 43s —— View job


Review

  • Gather context (diff, changed files)
  • Review code changes
  • Post review

Went through the diff (origin/devel...HEAD) in detail. Overall this is a clean, well-tested change — the three goals (export ImageArray class, derive axes from scales instead of a separate slot, flip the scale convention to 1/s) are implemented consistently across R/ImageArray.R, R/Validity.R, R/transformations.R, and R/manipulation.R, and the new tests in test-axes.R/test-scales.R cover the permutation and validation logic well.

A couple of minor points, nothing blocking:

  1. R/AllClasses.R:34ImageArray class is now @exportClass'd but still tagged @keywords internal, so it won't appear in the reference index. Since the whole point of exporting is downstream use by romeo, worth double-checking that's intentional rather than an oversight.
  2. R/ImageArray.R axes<- — the replacement only permutes the scales metadata (via scales(object)[value]), it does not reorder the underlying array data in @levels. That matches the old behavior (metadata-only), but could be worth a doc note since it's a common point of confusion (axes labels vs. physical dimension order).
  3. Style nit: R/Validity.R dropped braces from several if (...) stop(...) statements — fine as-is, just flagging since it's a slight style shift from the rest of the file.

No correctness issues found in the scale-direction flip (createListFromEBImage, .magick_resize_scale, .get_scales_from_dim, .get_scales_from_nlevels, affine/scale/crop in transformations.R/manipulation.R) — all consistently updated to the new 1/s convention and backed by matching test updates.

Note: no R environment available in this sandbox, so this is a static review only (couldn't run devtools::test() or R CMD check).

@Artur-man
Artur-man merged commit 81809f2 into devel Aug 10, 2026
10 of 12 checks passed
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.

1 participant