Skip to content
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.

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

tiledb_dim() prints argument 'filter_list' as 'filters' #678

Closed
cgiachalis opened this issue Mar 22, 2024 · 4 comments · Fixed by #681
Closed

tiledb_dim() prints argument 'filter_list' as 'filters' #678

cgiachalis opened this issue Mar 22, 2024 · 4 comments · Fixed by #681

Comments

@cgiachalis
Copy link
Contributor

This is not functional issue but the print of tiledb_dim object is not matching the construction; 'filter_list' argument appears as 'filters'. This is traced back to an internal function `.as_text_dimension().

library(tiledb)
# TileDB R 0.25.0 with TileDB Embedded 2.21.0 on Windows 10 x64 (build 19045).

tiledb_dim(
  name = "a",
  domain = c(1L, 10L),
  tile = 10L,
  type = "INT32",
  filter_list = tiledb_filter_list(c(
    tiledb_filter_set_option(tiledb_filter("ZSTD"), "COMPRESSION_LEVEL", -1)
  )))
#> tiledb_dim(name="a", domain=c(1L,10L), tile=10L, type="INT32", filters=tiledb_filter_list(c(tiledb_filter_set_option(tiledb_filter("ZSTD"),"COMPRESSION_LEVEL",-1))))
#>                                                                      ^----- here

# attr looks OK
tiledb_attr(
  name = "model",
  type = "INT32",
  ncells = 1,
  nullable = FALSE,
  filter_list = tiledb_filter_list(c(
    tiledb_filter_set_option(tiledb_filter("ZSTD"), "COMPRESSION_LEVEL", -1)
  ))
) |> show() # it doesn't print; we're doing manually
#> tiledb_attr(name="model", type="INT32", ncells=1, nullable=FALSE, filter_list=tiledb_filter_list(c(tiledb_filter_set_option(tiledb_filter("ZSTD"),"COMPRESSION_LEVEL",-1))))

txt <- paste0(txt, "filters=", .as_text_filter_list(fl), ")")

For attributes:

if (nfilters(fl) > 0) paste0(", filter_list=", .as_text_filter_list(fl)),

On top of that, a tiny typo was found : ?tiledb_dim and ?tiledb_attr (hint: Contructs -> Constructs)

Thanks

@eddelbuettel
Copy link
Contributor

Thanks! And really appreciate your attentiveness to detail here. Would you want to propose a PR to correct either or both shortcomings?

@cgiachalis
Copy link
Contributor Author

Sure! It might good chance to address another one I have in my notes (doc related). I will send a PR over the weekend.

@eddelbuettel
Copy link
Contributor

I also have not forgotten about your open (and dated, by my inaction) issue about 0- and 1- indexing. It's a tricky one!

@cgiachalis
Copy link
Contributor Author

Oh, thanks - I thought about it back then and concluded that needs clear documentation wherever it needs one; for example there is one function that has one argument that accepts (0-based) and another with (1-based). But it's not a deal if it's documented it.

But there is something else that I am about to open soon and is similar to #524.

eddelbuettel pushed a commit that referenced this issue Mar 23, 2024
* Fix title typo in 'tiledb_dim' and 'tiledb_attr'

* Set correct argument in returning text of 'tiledb_dim' method

Closes #678

* Consistent use of "must be" in error messages

* Capital letter for the first word in titles

* Correct description of uri param
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 a pull request may close this issue.

2 participants