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

(chore): Clean up AnnDataWrapper API #341

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

ilan-gold
Copy link
Collaborator

Open to other options here, but basically I did two things.

  1. adata_XXX -> base_XXX since the semantics are a bit more general and I think adata is redundant anyway
  2. path -> elem since I think elem is a bit nicer and path can refer to a sub-URL which is kind of a path, but also not really.

This is definitely a breaking change, so I'm going to try to finish up other PRs that are also breaking at the same time, like our python support.

@keller-mark
Copy link
Member

While I agree it may not be the nicest API, without more context/motivation, I don't think there is enough to justify this breaking change, as it will break existing docs/reference materials, Vitessce config generation code in portal-visualization package, and notebooks that are being used in HuBMAP workspaces that rely on the VitessceConfig.to_python function

@ilan-gold
Copy link
Collaborator Author

@keller-mark This is motivated by #333. We could just keep the non-outward-facing changes so that when the SpatialDataWrapper calls "private" attributes, they have better names.

Comment on lines +10 to +19
# Ignore line too long
E501,
# Ignore invalid escape sequence '\*'
W605,
# Ignore line break before binary operator: Skim down the left edge to understand intent.
W503,
# Ignore continuation line over-indented for visual indent
E127
# Ignore continuation line under-indented for visual indent
E128
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ilan-gold ilan-gold marked this pull request as ready for review August 30, 2024 14: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.

2 participants