Skip to content

remove extra forward slashes #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Artur-man wants to merge 1 commit into
base: devel
Choose a base branch
from
Open

remove extra forward slashes #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion R/ZarrSparseMatrixSeed-class.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ read_sparse_zarr_component <- function(zarr_store, group, name,
if (!zarr_exists(zarr_store, group))
stop(wmsg("Group \"", group, "\" does not exist in this Zarr store"))
if (zarr_node_is_dataset(zarr_store, group)) {
is_X_or_layer <- group == "/X" || startsWith(group, "/layers/")

@hpages hpages Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is a good example of why normalization is important. If we know that group is guaranteed to always start with a single /, then the current code does the right thing. If we know that group is guaranteed to never start with a /, then the current code no longer does the right thing but is easy to fix.
But if group sometimes has the leading slash and sometimes not, then both, the current code and the proposed change are wrong.

Additionally, the proposed change is wrong for another reason: this code tries to recognize datasets that belong to the /layers group (in which case the dataset names are going to be of the form /layers/<dataset_name> e.g. /layers/counts), but the proposed change will recognize datasets that belong to groups that start with layers (e.g. layersalt), which is not what we want.

is_X_or_layer <- group == "X" || startsWith(group, "layers")
msg1 <- c("\"", group, "\" is a Zarrr dataset, not a Zarr group, ",
"so it looks like the matrix that you are trying to ",
"access is not stored in a sparse format. Please ",
Expand Down
2 changes: 0 additions & 2 deletions R/utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,6 @@ normarg_zarr_group <- function(name, what1="'name'",
stop(wmsg(what1, " cannot be the empty string"))
if (startsWith(name, "/")) {
name <- sub("^/*", "/", name) # only keep first leading slash
} else {
name <- paste0("/", name)

@hpages hpages Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The purpose of normarg_zarr_group() is to normalize the supplied group, but the proposed change no longer does that. I'm open to discuss whether the normalized group should have a leading slash or not. In the current normarg_zarr_group(), the decision was made to always have a leading slash, whether the user supplied it or not. However, the proposed change no longer makes any decision with respect to the leading slash: it keeps it if the user supplied it but it doesn't add it if the user didn't. So it doesn't normalize the group anymore.

Personally I have a small preference for having the leading slash in the normalized group. This is consistent with what we do with the new group() method in HDF5Array which in turn is consistent with what they do in rhdf5::h5ls(). More generally speaking I think it's important to try to keep things as consistent as possible across HDF5Array and ZarrArray.

BTW, I'm about to add a group() method in ZarrArray that will return the normalized group of a ZarrSparseMatrixSeed or ZarrSparseMatrix object.

}
name
}
Expand Down
2 changes: 1 addition & 1 deletion tests/testthat/test-writeZarrArray.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ test_that("ZarrRealizationSink()", {
sink <- ZarrRealizationSink(c(85, 20, 300), zarr_version=zarr_version)
expect_true(is(sink, "ZarrRealizationSink"))
expect_true(is(sink, "RealizationSink"))
format <- ZarrArray:::get_zarr_format(paste0(sink@zarr_path, "/"))
format <- ZarrArray:::get_zarr_format(sink@zarr_path)

@hpages hpages Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

get_zarr_format() is a thin wrapper to Rarr:::.read_array_metadata() and IIRC earlier versions of the latter required the trailing slash. Happy to get rid of it.

expect_identical(format, zarr_version)
seed <- as(sink, "ZarrArraySeed")
expect_true(is(seed, "ZarrArraySeed"))
Expand Down