theislab / scib

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

Update to scvi tools again, also scgen which now uses scvi tools #316

Closed adamgayoso closed 2 years ago

adamgayoso commented 2 years ago

This also correctly pipes the hvg argument to be used for scvi-tools models. Not sure what is correct here.

mumichae commented 2 years ago

Hi @adamgayoso ,

Thanks a lot for looking into this. Could you update this PR to the latest main so that we hopefully get a green build?

codecov[bot] commented 2 years ago

Codecov Report

Merging #316 (6771780) into main (9ecda46) will increase coverage by 0.93%. The diff coverage is 11.53%.

@@            Coverage Diff             @@
##             main     #316      +/-   ##
==========================================
+ Coverage   54.22%   55.16%   +0.93%     
==========================================
  Files          36       36              
  Lines        2001     1967      -34     
==========================================
  Hits         1085     1085              
+ Misses        916      882      -34     
Flag Coverage Δ
unittest 55.16% <11.53%> (+0.93%) :arrow_up:

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

Impacted Files Coverage Δ
scib/integration.py 19.01% <11.53%> (+3.28%) :arrow_up:
mumichae commented 2 years ago

Thanks!

Just a minor thing, could you also run pre-commit to fix file formatting? i.e.

pre-commit run --all-files 
LuckyMD commented 2 years ago

@mumichae could we include a pre-commit run into the CI pipeline to automate a check and run of pre-commit?

mumichae commented 2 years ago

Pre-commit is included in the CI, so it currently fails

LuckyMD commented 2 years ago

Yes, I meant a pre-commit run and commit like here

mumichae commented 2 years ago

@mumichae could we include a pre-commit run into the CI pipeline to automate a check and run of pre-commit?

@LuckyMD I'm not quite sure if having Github actions create new commits upon linting errors is such a good idea. This could potentially lead to merge issues when one forgets to pull those commits before creating new commits. It would be easier to have anyone developing to use pre-commit locally and/or have maintainers update syntax formatting to any PR branches (as I did in this case)

mumichae commented 2 years ago

Thanks @adamgayoso for adapting these functions. I checked that the code runs through in scib-pipeline, so I'm merging this PR.

LuckyMD commented 2 years ago

Hmm, git should be smart enough to tell the dev that they are behind the PR head and automatically resolve the merge conflicts if it's only linting, no?