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

Bugfix: relax the requirements #154

Merged
merged 5 commits into from
Jul 28, 2020
Merged

Bugfix: relax the requirements #154

merged 5 commits into from
Jul 28, 2020

Conversation

denproc
Copy link
Collaborator

@denproc denproc commented Jul 27, 2020

Closes #143
The problem was related to the upgrade of the environment with torch incompatible with CUDA.

Proposed Changes

  • made requirements on pytorch and torchvision less strict
  • added warning to the BRISQUE score that backprop is not available using `torch==1.5.0'
  • added notes to the docstring and readme

@denproc denproc requested review from zakajd and snk4tr July 27, 2020 13:36
@codecov
Copy link

codecov bot commented Jul 27, 2020

Codecov Report

Merging #154 into master will decrease coverage by 0.05%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #154      +/-   ##
==========================================
- Coverage   94.64%   94.59%   -0.06%     
==========================================
  Files          24       24              
  Lines        1624     1627       +3     
==========================================
+ Hits         1537     1539       +2     
- Misses         87       88       +1     
Flag Coverage Δ
#unittests 94.59% <66.66%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
piq/brisque.py 99.04% <66.66%> (-0.96%) ⬇️

@denproc
Copy link
Collaborator Author

denproc commented Jul 27, 2020

Ready to merge after #153

zakajd
zakajd previously approved these changes Jul 27, 2020
Copy link
Collaborator

@zakajd zakajd left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@snk4tr snk4tr left a comment

Choose a reason for hiding this comment

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

I am afraid this fix will not help with the problem of different CUDA versions, which was discussed offline

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@denproc denproc marked this pull request as draft July 27, 2020 16:33
@denproc
Copy link
Collaborator Author

denproc commented Jul 27, 2020

Let me look into it a bit more after @snk4tr comment

@denproc denproc marked this pull request as ready for review July 28, 2020 05:25
@denproc
Copy link
Collaborator Author

denproc commented Jul 28, 2020

@snk4tr @zakajd the proposed approach works, ready for review.

As I suggested, requirement torch>=1.2 covers the tags used to specify the cuda driver version.

Additional context just in case:

[In]: pip list
[Out]: torch                  1.5.0+cu101 
[Out]: torchvision            0.6.0+cu101 

[In]: python setup.py install
[In]: pip list
[Out]: piq                    0.5.1    
[Out]: torch                  1.5.0+cu101 
[Out]: torchvision            0.6.0+cu101 

Copy link
Contributor

@snk4tr snk4tr left a comment

Choose a reason for hiding this comment

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

Great!
Merging 👍

@snk4tr snk4tr merged commit 3db33e3 into master Jul 28, 2020
@denproc denproc deleted the bug/requirements branch July 28, 2020 14:57
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.

[Query] Why is torch 1.5.0 imposed?
3 participants