search

theislab / scib

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

Bump rp2y dependency #334

Closed mumichae closed 2 years ago

mumichae commented 2 years ago

Unpin rpy2 dependency and make sure test run through without failing. This will close #322

Skipping pytest on macos (due to unresolved segmentation fault)

codecov[bot] commented 2 years ago

Codecov Report

Merging #334 (79d7599) into main (067eb1a) will increase coverage by 30.79%. The diff coverage is n/a.

@@             Coverage Diff             @@
##             main     #334       +/-   ##
===========================================
+ Coverage   25.90%   56.70%   +30.79%     
===========================================
  Files          38       38               
  Lines        1984     1984               
===========================================
+ Hits          514     1125      +611     
+ Misses       1470      859      -611
Flag Coverage Δ
unittest 56.70% <ø> (+30.79%) :arrow_up:

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

Impacted Files Coverage Δ
scib/metrics/clustering.py 82.00% <0.00%> (+2.00%) :arrow_up:
scib/utils.py 68.18% <0.00%> (+4.54%) :arrow_up:
scib/preprocessing.py 21.30% <0.00%> (+6.80%) :arrow_up:
scib/metrics/kbet.py 22.68% <0.00%> (+7.21%) :arrow_up:
tests/common.py 51.35% <0.00%> (+13.51%) :arrow_up:
scib/metrics/lisi.py 45.30% <0.00%> (+34.25%) :arrow_up:
tests/metrics/test_kbet.py 100.00% <0.00%> (+42.85%) :arrow_up:
tests/metrics/test_graph_connectivity.py 100.00% <0.00%> (+50.00%) :arrow_up:
tests/conftest.py 100.00% <0.00%> (+54.83%) :arrow_up:
tests/preprocessing/test_clustering.py 100.00% <0.00%> (+60.00%) :arrow_up:
... and 17 more
Zethson commented 2 years ago

I don't see where the PR unpins rpy2?

mumichae commented 2 years ago

That was done on main (which is bad Ik) and then the CI broke. I was just cleaning up the aftermath that could have been related to the version bump. Should I perhaps rename the PR?

Zethson commented 2 years ago

That was done on main (which is bad Ik) and then the CI broke. I was just cleaning up the aftermath that could have been related to the version bump. Should I perhaps rename the PR?

Ahhh, no worries. I usually recommend to add a reason for why you're pinning something (igraph in this case) IF you're not pinning everything (would be the alternative). Can later help figure out why igraph<0.10 was required in the first place. If this pin is not necessary consider getting rid of it :)

mumichae commented 2 years ago

igraph 0.10 was only released today and the louvain functions was throwing errors, so I pinned it. However, I see that leidenalg also restricts to igraph<0.10, so in that case igraph no longer needs to be pinned