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

Make rownames part of obs and var #171

Merged
merged 13 commits into from
Jul 8, 2024
Merged

Make rownames part of obs and var #171

merged 13 commits into from
Jul 8, 2024

Conversation

rcannood
Copy link
Collaborator

@rcannood rcannood commented Jul 4, 2024

These changes have been ported from #166 and #169.

Rownames have been moved from a separate obs_names and var_names to being the rownames of obs and var, since it simplifies the way the data is read to and from an hdf5 file.

@rcannood rcannood requested a review from LouiseDck July 4, 2024 22:05
Copy link
Collaborator

@LouiseDck LouiseDck left a comment

Choose a reason for hiding this comment

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

I have no issues with the way you refactored obs_names and var_names.

If I recall correctly though, the reason for not doing it this way in the first place was the following:
rownames in R can only be character vectors, which is an assumption that does not (or will not) apply to the Python anndata version. I believe that the impact of this is relatively minor, however I do feel it is necessary to be documented somewhere?
(This is in general: we need to have some documentation delineating the differences and what is and isn't supported)

@rcannood
Copy link
Collaborator Author

rcannood commented Jul 8, 2024

Good idea! I was going to add it to the roxygen docs, but found it a bit weird to document differences between R and Python while the Python AnnData also only allows string obs_names and var_names for now. I'll create a separate issue.

@lazappi
Copy link
Collaborator

lazappi commented Jul 8, 2024

I have no issues with the way you refactored obs_names and var_names.

If I recall correctly though, the reason for not doing it this way in the first place was the following: rownames in R can only be character vectors, which is an assumption that does not (or will not) apply to the Python anndata version. I believe that the impact of this is relatively minor, however I do feel it is necessary to be documented somewhere? (This is in general: we need to have some documentation delineating the differences and what is and isn't supported)

You are right. I had forgotten but this was (at least partly) why we changed it before. I would probably make a special man page and/or vignette for things we know are different from Python.

@rcannood
Copy link
Collaborator Author

rcannood commented Jul 8, 2024

I would probably make a special man page and/or vignette for things we know are different from Python.

Good point! I added it to #172 ☺️

@rcannood rcannood merged commit 220f977 into main Jul 8, 2024
7 checks passed
@rcannood rcannood deleted the rework_rownames branch July 8, 2024 06:23
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