Skip to content

[r] Fix DenseNDArray write after create#2970

Merged
johnkerl merged 1 commit intomainfrom
kerl/dense-writeable-after-create
Sep 9, 2024
Merged

[r] Fix DenseNDArray write after create#2970
johnkerl merged 1 commit intomainfrom
kerl/dense-writeable-after-create

Conversation

@johnkerl
Copy link
Copy Markdown
Contributor

@johnkerl johnkerl commented Sep 9, 2024 •
edited
Loading

Issue and/or context: Found while debugging an issue with #2950 for issue #2407 / [sc-51048].

Changes:

Make it possible to write to a DenseNDArray using an open-for-write after the array is created.

Notes for Reviewer:

Note that the create methods return an array opened for write.

For SparseNDArray and DenseNDArray you can do:

  • create
  • write data
  • close the handle from create

For SparseNDArray, you can do:

  • create
  • close the handle from create
  • open for write
  • write data
  • close the handle from open

For DenseNDArray, this does not work before this PR. The error message is

Error: soma_data must be a DataType, not NULL

You can reproduce this by including in your checkout the mod to apis/r/tests/testthat/test-SOMADenseNDArray.R on this PR but excluding the mod to apis/r/R/SOMADenseNDArray.R on this PR, then testing, e.g.:

> devtools::test_active_file(file="tests/testthat/test-SOMADenseNDArray.R")
TileDB-SOMA R package 1.13.99.6 with TileDB Embedded 2.25.0 on macOS Sonoma 14.6.1.
See https://github.com/single-cell-data for more information about the SOMA project.

══ Testing test-SOMADenseNDArray.R ═════════════════════════════════════════════════════════════════════════════════════
[ FAIL 1 | WARN 0 | SKIP 0 | PASS 39 ]

── Error (test-SOMADenseNDArray.R:18:3): SOMADenseNDArray creation ─────────────
Error: soma_data must be a DataType, not NULL
Backtrace:
    ▆
 1. └─ndarray$write(mat) at test-SOMADenseNDArray.R:18:3
 2.   ├─arrow::schema(arrow::field("soma_data", private$.type)) at r/R/SOMADenseNDArray.R:138:7
 3.   │ └─rlang::list2(...)
 4.   └─arrow::field("soma_data", private$.type)
 5.     └─arrow:::as_type(type, name)
[ FAIL 1 | WARN 0 | SKIP 0 | PASS 39 ]

Including this PR's bugfix on apis/r/tests/testthat/test-SOMADenseNDArray.R, one sees

> devtools::test_active_file(file="tests/testthat/test-SOMADenseNDArray.R")
TileDB-SOMA R package 1.13.99.6 with TileDB Embedded 2.25.0 on macOS Sonoma 14.6.1.
See https://github.com/single-cell-data for more information about the SOMA project.

══ Testing test-SOMADenseNDArray.R ═════════════════════════════════════════════════════════════════════════════════════
[ FAIL 0 | WARN 0 | SKIP 0 | PASS 52 ] Done!

The root cause is that private$.type is set at create time but not recovered on the open-for-write. This bug was fixed here for SOMASparseNDArray on PR #2755; here we apply that same fix to DenseNDArray.

@johnkerl johnkerl force-pushed the kerl/dense-writeable-after-create branch from 2a5b57c to b491536 Compare September 9, 2024 13:14
@johnkerl johnkerl merged commit f6af002 into main Sep 9, 2024
@johnkerl johnkerl deleted the kerl/dense-writeable-after-create branch September 9, 2024 14:52
@johnkerl
Copy link
Copy Markdown
Contributor Author

johnkerl commented Sep 9, 2024

Thanks @mojaveazure !

This was referenced Sep 9, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mojaveazure mojaveazure mojaveazure approved these changes

@eddelbuettel eddelbuettel Awaiting requested review from eddelbuettel

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@johnkerl @mojaveazure