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

BUMP deepNOG to 1.1.0 (targeting PyPI) and update CI configurations #21

Merged

Conversation

LokiLuciferase
Copy link
Collaborator

Just what it says on the tin. Plus, some additional refactoring and added comments.

@LokiLuciferase LokiLuciferase requested review from VarIr and phyden March 7, 2020 00:05
@LokiLuciferase
Copy link
Collaborator Author

It is my understanding that we currently determine coverage from the AppVeyor CI build - is it easily possible to use the linux build of Travis instead @VarIr?

Copy link

@VarIr VarIr left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I see no blockers here, just some minor comments.

phenotrex/structure/records.py Outdated Show resolved Hide resolved
phenotrex/structure/records.py Outdated Show resolved Hide resolved
phenotrex/structure/records.py Outdated Show resolved Hide resolved
phenotrex/structure/records.py Show resolved Hide resolved
phenotrex/structure/records.py Outdated Show resolved Hide resolved
phenotrex/transforms/annotation.py Show resolved Hide resolved
phenotrex/transforms/annotation.py Outdated Show resolved Hide resolved
tests/test_ml.py Outdated Show resolved Hide resolved
phenotrex/util/plotting.py Show resolved Hide resolved
Copy link

@VarIr VarIr left a comment

Choose a reason for hiding this comment

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

Just saw that all Travis builds failed. While the Linux builds fail due to the same issue we see in deepnog, the mac builds fail due to something wrong with prodigal. These should be investigated.

@VarIr
Copy link

VarIr commented Mar 7, 2020

About coverage: It seems to me, we actually run codecov on Travis, but not on AppVeyor? In any case, it should be simple to enable it on any platform. codecov claim, they automatically merge reports from different platforms. So perhaps just add a line "codecov" to .appveyor.yml after the tests (and pytest --cov, unless we already have it).

@VarIr
Copy link

VarIr commented Mar 7, 2020

Btw, see the deepnog .appveyor.yml for how to obtain torch on Windows. (Hasn't found its way into setup.py, yet).

@LokiLuciferase
Copy link
Collaborator Author

Just saw that all Travis builds failed. While the Linux builds fail due to the same issue we see in deepnog, the mac builds fail due to something wrong with prodigal. These should be investigated.

Yeah, it was possible that the prodigal binary compiled for linux wouldn't run under macos (and it sure as hell won't under Win10). I can look into having it compile from source under linux/macos, and ship the windows binary with the repo.

@LokiLuciferase
Copy link
Collaborator Author

  • I have now added all prodigal binaries for linux, win32 and darwin architectures. I had to adapt some of the protein calling code since the pre-compiled packages don't allow passing gzipped files.
  • I also changed the license of phenotrex to GPL v3 to comply with the license of prodigal.
  • Finally, I added some more tests for file I/O and added a groups file for Sulfate_reducer to be able to test LOGO CV.

@codecov
Copy link

codecov bot commented Mar 7, 2020

Codecov Report

Merging #21 into master will increase coverage by 11.00%.
The diff coverage is 91.80%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #21       +/-   ##
===========================================
+ Coverage   71.76%   82.76%   +11.00%     
===========================================
  Files          21       21               
  Lines        1098     1120       +22     
===========================================
+ Hits          788      927      +139     
+ Misses        310      193      -117     
Impacted Files Coverage Δ
phenotrex/util/taxonomy.py 0.00% <ø> (ø)
phenotrex/structure/records.py 70.96% <50.00%> (-4.90%) ⬇️
phenotrex/ml/shap_handler.py 76.37% <80.00%> (+0.78%) ⬆️
phenotrex/ml/prediction.py 58.97% <83.33%> (-1.64%) ⬇️
phenotrex/ml/cccv.py 92.39% <100.00%> (ø)
phenotrex/ml/feature_select.py 95.16% <100.00%> (+6.92%) ⬆️
phenotrex/transforms/annotation.py 98.66% <100.00%> (+84.63%) ⬆️
phenotrex/util/plotting.py 88.88% <100.00%> (+0.20%) ⬆️
phenotrex/transforms/__init__.py 60.00% <0.00%> (-40.00%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02da355...c2ece98. Read the comment docs.

Copy link

@VarIr VarIr left a comment

Choose a reason for hiding this comment

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

It's good to see the coverage go up! I'd prefer to see the gene calling in a context manager, if possible, but otherwise LGTM.

LICENSE Outdated Show resolved Hide resolved
phenotrex/structure/records.py Outdated Show resolved Hide resolved
phenotrex/transforms/annotation.py Outdated Show resolved Hide resolved
@VarIr
Copy link

VarIr commented Mar 8, 2020

Btw, the Travis errors will likely go away with PyTorch 1.5. We might want to allow Travis to fail the Linux builds until then (or simply ignore the errors).

@LokiLuciferase
Copy link
Collaborator Author

Merging this now. @VarIr maybe you can upload to pypi when you find time?

@LokiLuciferase LokiLuciferase merged commit 204f8b1 into univieCUBE:master Mar 11, 2020
@LokiLuciferase LokiLuciferase deleted the feature-deepnog-update branch March 11, 2020 20:13
@VarIr
Copy link

VarIr commented Mar 12, 2020

Thank you for the PR! I've just created a new release and uploaded to PyPI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants