theislab / scib

Benchmarking analysis of data integration tools
MIT License
294 stars 63 forks source link

Feat/add moransi #304

Open MxMstrmn opened 2 years ago

MxMstrmn commented 2 years ago

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

A new approach to integrate #245

MxMstrmn commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

MxMstrmn commented 2 years ago

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

michalk8 commented 2 years ago

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 commented 2 years ago

Thanks so much, @michalk8!

mumichae commented 2 years ago

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 commented 2 years ago

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:

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

mumichae commented 2 years ago

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 commented 2 years ago

Updated version of #303

codecov[bot] commented 2 years ago

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%) :arrow_up:

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%) :arrow_up:
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%) :arrow_up:
tests/metrics/test_morans_i.py 100.00% <100.00%> (ø)
scib/utils.py 74.46% <0.00%> (+12.76%) :arrow_up: