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

Extend roundtrip tests #107

Merged
merged 32 commits into from
Dec 2, 2023
Merged

Extend roundtrip tests #107

merged 32 commits into from
Dec 2, 2023

Conversation

rcannood
Copy link
Collaborator

@rcannood rcannood commented Sep 18, 2023

Changes:

  • Cover different matrix types: numeric_matrix, numeric_csparse, numeric_rsparse, numeric_matrix_with_nas, ...
  • Cover different vector types: character, integer, factor, factor_ordered, logical, numeric, character_with_nas, ...
  • Split up hdf5-reticulate roundtrip tests to more easily find out which data types are causing issues
  • Incorporate regular write_h5ad-read_h5ad tests by @ivirshup
  • Dataset generator functions are no longer exported, as people shouldn't be using these, and we want to be able to change the interface in order to perform rigorous testing.

Note:

  • Failing tests are disabled and will be re-enabled in a separate PR
  • The dataset generator will probably be re-exported in a different PR

@rcannood rcannood changed the title add more types to dummy data refactor dataset generators Sep 20, 2023
@rcannood rcannood changed the title refactor dataset generators extend hdf5-reticulate roundtrip tests Sep 20, 2023
@rcannood
Copy link
Collaborator Author

@ivirshup Is it okay that I split up your roundtrip tests into different files?

I split them up because if something went wrong during a roundtrip test, it was hard to know which slot was causing the issue.

Also, while test-roundtrip.R tested both HDF5AnnData and InMemoryAnnData, I'm only testing HDF5AnnData here because using the InMemoryAnnData would still first read everything out with HDF5AnnData and then convert it to an InMemoryAnnData. It may be better to simply unit test the to_InMemoryAnnData function separately.

@rcannood rcannood changed the title extend hdf5-reticulate roundtrip tests Extend roundtrip tests Sep 20, 2023
@ivirshup
Copy link
Member

@ivirshup Is it okay that I split up your roundtrip tests into different files?

👍

Also, while test-roundtrip.R tested both HDF5AnnData and InMemoryAnnData, I'm only testing HDF5AnnData here because using the InMemoryAnnData would still first read everything out with HDF5AnnData and then convert it to an InMemoryAnnData. It may be better to simply unit test the to_InMemoryAnnData function separately.

Up to you. I found this quite useful since it's shared assumptions across tests. Plus this is something that should be true in both cases.

If the InMemory case fails and the HDF5 one doesn't, then it was easy to tell the issue was with conversion of InMemory to HDF5

rcannood added a commit that referenced this pull request Nov 30, 2023
* use validate aligned array instead of matrix

* refactor validate_aligned_array

* update package docs

* add has_row_names to utils

* skip seurat converter for now

* remove outdated tests

* refactor roundtrip tests

* remove roundtrip tests (will be added back in #107)

* add extra line

* comment out seurat object example
@rcannood rcannood merged commit f9e8a9a into main Dec 2, 2023
7 checks passed
@rcannood rcannood deleted the more_types_to_dummy_data branch December 2, 2023 09:29
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.

2 participants