Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

standardize docstrings #30

Merged
merged 4 commits into from
Oct 14, 2022
Merged

standardize docstrings #30

merged 4 commits into from
Oct 14, 2022

Conversation

adamgayoso
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Oct 11, 2022

Codecov Report

Merging #30 (4fe7862) into main (e189453) will increase coverage by 0.13%.
The diff coverage is 95.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #30      +/-   ##
==========================================
+ Coverage   94.31%   94.44%   +0.13%     
==========================================
  Files          11       12       +1     
  Lines         422      432      +10     
==========================================
+ Hits          398      408      +10     
  Misses         24       24              
Flag Coverage Δ
unittests 94.44% <95.83%> (+0.13%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/scib_metrics/_isolated_labels.py 97.05% <ø> (ø)
src/scib_metrics/_silhouette.py 96.29% <ø> (ø)
src/scib_metrics/utils/_dist.py 100.00% <ø> (ø)
src/scib_metrics/utils/_silhouette.py 98.57% <ø> (ø)
src/scib_metrics/_utils.py 87.50% <87.50%> (ø)
src/scib_metrics/_ari_nmi.py 94.11% <100.00%> (+1.96%) ⬆️
src/scib_metrics/_lisi.py 94.59% <100.00%> (+0.30%) ⬆️

@adamgayoso adamgayoso requested a review from justjhong October 11, 2022 19:22
Copy link
Contributor

@justjhong justjhong left a comment

Choose a reason for hiding this comment

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

X feels weird to me (thinking more adj_mtx, but also for LISI it would bug out on non-KNN adj mtxes bc there would be uneven number of neighbors), but u do u

import numpy as np
import scipy.sparse as sp

ArrayLike = Union[np.ndarray, sp.spmatrix, jnp.ndarray]
Copy link
Contributor

Choose a reason for hiding this comment

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

theres a types file for this i thought

@adamgayoso
Copy link
Member Author

You are right we should have one variable name for knn graphs and one for embeddings

Maybe we should use embedding and neighbor_distances and neighbor_connectivities explictly?

@adamgayoso adamgayoso enabled auto-merge (squash) October 14, 2022 00:58
@adamgayoso adamgayoso mentioned this pull request Oct 14, 2022
@adamgayoso adamgayoso merged commit cf2fea2 into main Oct 14, 2022
@adamgayoso adamgayoso deleted the standardize branch October 14, 2022 02:02
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants