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

Feat/add moransi #304

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

Feat/add moransi #304

wants to merge 17 commits into from

Conversation

MxMstrmn
Copy link
Contributor

@MxMstrmn MxMstrmn commented May 2, 2022

Updated version of moran's I PR with pre-commits

A new approach to integrate #245

@MxMstrmn MxMstrmn requested a review from mumichae May 2, 2022 20:21
@MxMstrmn
Copy link
Contributor Author

MxMstrmn commented May 2, 2022

Hi @michalk8, do you have an idea why the checks fail here? The file locally has no syntax errors and also passes the pre-commits I added as part of this PR. Do not know how to handle this best / fix it.

@michalk8
Copy link

michalk8 commented May 2, 2022

Done. Also, I'd seriously consider refactoring the CI to be more up-to-date.
Interestingly, AST checker pre-commit did not catch this.

@MxMstrmn
Copy link
Contributor Author

MxMstrmn commented May 2, 2022

Why does f"{variable=}" fail the parse, is that not normal python syntax?

@MxMstrmn
Copy link
Contributor Author

MxMstrmn commented May 2, 2022

I did not set the CI up, I guess @mumichae and I would be very glad for your input to improve this :)

@michalk8
Copy link

michalk8 commented May 2, 2022

Why does f"{variable=}" fail the parse, is that not normal python syntax?

It is, just from Python 3.8 (would deprecate 3.7 since numpy support ended last December), hence the refactoring.

@MxMstrmn
Copy link
Contributor Author

MxMstrmn commented May 2, 2022

Thanks so much, @michalk8!

@mumichae
Copy link
Collaborator

mumichae commented May 2, 2022

Done. Also, I'd seriously consider refactoring the CI to be more up-to-date.
Interestingly, AST checker pre-commit did not catch this.

Are you referring to the .github/workflow content to be updated based on a newer configuration template? I wasn't aware there was an update, what consequences would the current code have of not updated?

Would be happy to update, however that would be in a separate PR.

@michalk8
Copy link

michalk8 commented May 3, 2022

Are you referring to the .github/workflow content to be updated based on a newer configuration template? I wasn't aware there was an update, what consequences would the current code have of not updated?

Would do the following:

  • remove 3.7 CI, add 3.9 (unless you need to support it)
  • use pre-commits (add e.g. black) instead of running raw flake8 (this PR was)
  • add tests coverage
  • not sure how much work, but macOS test CI would be nice to have

The first 2 points I'd say are the one to focus on.

@mumichae
Copy link
Collaborator

mumichae commented May 3, 2022

Thanks a lot for the suggestions @michalk8 !

I think we should definitely make things more consistent, if we start using pre-commit (which we hadn't before). But I think this should be separated from Moran's I. I'll open a new issue with a description of what we'd need for pre-commit integration and would suggest we get that merged before the new metric.

@MxMstrmn
Copy link
Contributor Author

MxMstrmn commented May 3, 2022

Updated version of #303

@MxMstrmn MxMstrmn mentioned this pull request May 3, 2022
Move helper functions outside, only consider scores > 0, refactor code, reduce default n_hvg
@MxMstrmn MxMstrmn requested a review from LuckyMD May 3, 2022 12:50
@codecov
Copy link

codecov bot commented May 10, 2022

Codecov Report

Merging #304 (a8d2e08) into main (c993ffd) will increase coverage by 3.54%.
The diff coverage is 96.70%.

@@            Coverage Diff             @@
##             main     #304      +/-   ##
==========================================
+ Coverage   53.35%   56.90%   +3.54%     
==========================================
  Files          35       37       +2     
  Lines        1981     2072      +91     
==========================================
+ Hits         1057     1179     +122     
+ Misses        924      893      -31     
Flag Coverage Δ
unittest 56.90% <96.70%> (+3.54%) ⬆️

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

Impacted Files Coverage Δ
scib/preprocessing.py 30.05% <50.00%> (+8.50%) ⬆️
scib/metrics/morans_i.py 96.82% <96.82%> (ø)
scib/metrics/__init__.py 100.00% <100.00%> (ø)
scib/metrics/metrics.py 82.29% <100.00%> (+0.97%) ⬆️
tests/metrics/test_morans_i.py 100.00% <100.00%> (ø)
scib/utils.py 74.46% <0.00%> (+12.76%) ⬆️

@mumichae mumichae added this to In progress in scib maintenance May 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
scib maintenance
In progress
Development

Successfully merging this pull request may close these issues.

None yet

3 participants