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

_spatial_norm_callback implementation #698

Closed
selmanozleyen opened this issue May 27, 2024 · 1 comment
Closed

_spatial_norm_callback implementation #698

selmanozleyen opened this issue May 27, 2024 · 1 comment
Labels
conceptual Conceptual discussions low-prio Low priority issue.

Comments

@selmanozleyen
Copy link
Collaborator

hi @giovp
I am writing this issue to document one of the commits I will make in #696. At this point it will be hard for me to separate that commit in the PR and it will be a small discussion compared to how big the PR will be. I want to discuss this implementation.

# TODO(@giovp): refactor
@staticmethod
def _spatial_norm_callback(
term: Literal["x", "y"],
adata: AnnData,
adata_y: Optional[AnnData] = None,
attr: Optional[Literal["X", "obsp", "obsm", "layers", "uns"]] = None,
key: Optional[str] = None,
) -> TaggedArray:
if term == "y":
if adata_y is None:
raise ValueError("When `term` is `y`, `adata_y` cannot be `None`.")
adata = adata_y
if attr is None:
raise ValueError("`attrs` cannot be `None` with this callback.")
spatial = TaggedArray._extract_data(adata, attr=attr, key=key)
logger.info(f"Normalizing spatial coordinates of `{term}`.")
spatial = (spatial - spatial.mean()) / spatial.std()
return TaggedArray(spatial, tag=Tag.POINT_CLOUD)

First Issue: Typo

first of all I will fix the type from attrs to attr

Second Issue: Standardization

but I also want to ask whether

spatial = (spatial - spatial.mean()) / spatial.std()

is a correct standardization, shouldn't it be

spatial = (spatial - spatial.mean(axis=0,keepdims=True)) / spatial.std(axis=0,keepdims=True)

since we'd like each feature to be gaussian units right? or am I missing something?

@selmanozleyen selmanozleyen added low-prio Low priority issue. conceptual Conceptual discussions labels May 27, 2024
@giovp
Copy link
Member

giovp commented May 27, 2024

hi @selmanozleyen , yes the axis=0 is correct for mean, not for std as it would introduce a distortion in the axis, I've added the fix in #697

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conceptual Conceptual discussions low-prio Low priority issue.
Projects
None yet
Development

No branches or pull requests

2 participants