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

H5ad helpers with pr83 #90

Merged
merged 9 commits into from
Sep 1, 2023
Merged

H5ad helpers with pr83 #90

merged 9 commits into from
Sep 1, 2023

Conversation

rcannood
Copy link
Collaborator

@rcannood rcannood commented Sep 1, 2023

Merges #83 into #88

mtmorgan and others added 9 commits May 12, 2023 08:46
- use `as.vector()` to reduce code duplication when reading
  nullable vectors
- closes #82
- 'quietly' to suppress warnings; set to TRUE when relevant warnings
  are handled by calling code
- versions of rhdf5 that don't support ENUM; more user-friendly message
- use to suppress irrelevant warnings in unit tests
* main: (26 commits)
  add gthb pat
  Update HDF5 writing functions and implement data frame writing (#85)
  remove shape trackstatus for now
  initial implementation of from_Seurat (#64)
  only check size when the names are already defined
  todo: add back obs_names and var_names length check
  update roxygen
  move required args to the front
  Update R/HDF5-read.R
  fix lint issues
  update tests
  refactor code to assume obs_names and var_names are defined
  Style and lint
  Add rhdf5 package skipe to HDF5 tests
  Add 1D sparse array to example.h5ad
  Remove requireNamespace("rhdf5")
  Fix bug in read_h5ad_string_array()
  Add support for reading H5AD rec arrays
  Covert 1D string arrays to vectors in read_h5ad_string_array()
  Update tests to use example.h5ad
  ...
if (is.null(ordered)) {
attributes <- rhdf5::h5readAttributes(file, name)
ordered <- attributes[["ordered"]]
if (is.na(ordered)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Without any testing I think this might need to be is.null() (like in the original line)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm. Assuming attributes is a named list not containing the key ordered, that would make sense. However, when I change is.na(ordered) to is.null(ordered) and run the tests again, I get the following error:

Error (test-HDF5AnnData.R:32:3): reading obs works
Error in `if (ordered) "ordered"`: argument is not interpretable as logical
Backtrace:
    ▆
 1. └─anndataR (local) `<fn>`() at test-HDF5AnnData.R:32:2
 2.   └─anndataR:::read_h5ad_element(private$.h5obj, "/obs", include_index = FALSE) at anndataR/R/HDF5AnnData.R:43:8
 3.     └─anndataR (local) read_fun(file = file, name = name, version = version, ...) at anndataR/R/HDF5-read.R:68:2
 4.       └─anndataR:::read_h5ad_collection(file, name, column_order) at anndataR/R/HDF5-read.R:369:2
 5.         └─anndataR:::read_h5ad_element(...) at anndataR/R/HDF5-read.R:424:4
 6.           └─anndataR (local) read_fun(file = file, name = name, version = version, ...) at anndataR/R/HDF5-read.R:68:2
 7.             └─base::factor(codes, labels = levels, ordered = ordered) at anndataR/R/HDF5-read

I can't even open the example hdf5 file:

> devtools::load_all()
> file <- system.file("extdata", "example.h5ad", package = "anndataR")
> adata <- HDF5AnnData$new(file)
> obs <- adata$obs
Error in if (ordered) "ordered" : 
  argument is not interpretable as logical

Copy link
Collaborator Author

@rcannood rcannood Sep 1, 2023

Choose a reason for hiding this comment

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

When I add print(attributes) before the if-statement, I see:

> adata <- HDF5AnnData$new(file)
> adata
$`encoding-type`
[1] "categorical"

$`encoding-version`
[1] "0.2.0"

$ordered
[1] NA

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Going to assume is.na(ordered) is the correct condition. If we find out that ordered can sometimes be NULL as well, we can always add a second condition.

@rcannood rcannood merged commit d8445a6 into h5ad-helpers Sep 1, 2023
6 checks passed
rcannood added a commit that referenced this pull request Sep 4, 2023
* Add read_h5ad function

* Add read_h5ad tests

* Add write_h5ad function

* Move test dummy helper to main function

Makes it possible to use for examples. Also add SingleCellExperiment and
Seurat dummy functions.

* Refactor dummy datasets into a single function

Needs to be exported for examples

* Add write_h5ad tests and examples

* Style and lint

* H5ad helpers ++ (#89)

* Remove temporary workaround

* use `inherits()` instead of `is()`

* Add HDF5AnnData and InMemoryAnnData to `read_h5ad()`

* fix incorrect merge

* try to fix error in R devel

* fix docs

* add terminal newline

* remove conditions

* undo previous commit

* H5ad helpers with pr83 (#90)

* support rhdf5 nullable vector and boolean enum implementation

- use `as.vector()` to reduce code duplication when reading
  nullable vectors
- closes #82

* implement and use read_h5ad_attributes

- 'quietly' to suppress warnings; set to TRUE when relevant warnings
  are handled by calling code

* use 'anndataR-category-unknown' as a class of warning

- versions of rhdf5 that don't support ENUM; more user-friendly message
- use to suppress irrelevant warnings in unit tests

* Tidy after merge and style

* More tidying...

* Roxygenise

* Fix namespace

---------

Co-authored-by: Martin Morgan <[email protected]>
Co-authored-by: Luke Zappia <[email protected]>

---------

Co-authored-by: Robrecht Cannoodt <[email protected]>
Co-authored-by: Martin Morgan <[email protected]>
@rcannood rcannood deleted the h5ad-helpers-with-pr83 branch September 19, 2023 04:20
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