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

MAINT redefine cdgmm3d to call cdgmm in torch skcuda backend #865

Merged
merged 2 commits into from
Jun 20, 2022

Conversation

MuawizChaudhary
Copy link
Collaborator

This pull request redefines cdgmm3d to call cdgmm in torch skcuda backend.

I'm also using this as a proxy for testing the torch_skcuda 3d backend.

@codecov-commenter
Copy link

Codecov Report

Merging #865 (442a5fb) into dev (4a6d9ea) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #865      +/-   ##
==========================================
+ Coverage   87.83%   88.02%   +0.18%     
==========================================
  Files          64       64              
  Lines        2253     2221      -32     
==========================================
- Hits         1979     1955      -24     
+ Misses        274      266       -8     
Flag Coverage Δ
jenkins_main 88.02% <100.00%> (+0.18%) ⬆️

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

Impacted Files Coverage Δ
...matio/scattering3d/backend/torch_skcuda_backend.py 100.00% <100.00%> (+19.04%) ⬆️

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 4a6d9ea...442a5fb. Read the comment docs.

@lostanlen
Copy link
Collaborator

is this tied to a specific issue? what's the plan here?

@MuawizChaudhary
Copy link
Collaborator Author

my reasoning: why are we repeating code that we already have?

@lostanlen
Copy link
Collaborator

oh wow, we have duplicate code here? then yes sounds like an easy win

@lostanlen
Copy link
Collaborator

is the change from a RuntimeError to a TypeError harmless here?

@MuawizChaudhary
Copy link
Collaborator Author

define harmless?

I changed it because it seemed like we forgot to switch the 3d cgmm over to throwing a Type Error when we changed the 1d and 2d code to be unified

@janden
Copy link
Collaborator

janden commented Jun 13, 2022

my reasoning: why are we repeating code that we already have?

Maybe there was a reason for having a separate cdgmm3d? On the other hand, the tests pass…

@MuawizChaudhary
Copy link
Collaborator Author

MuawizChaudhary commented Jun 14, 2022

my reasoning: why are we repeating code that we already have?

Maybe there was a reason for having a separate cdgmm3d? On the other hand, the tests pass…

The code is the same except for the error checking.

Only one test had to be changed in order for the tests to be passing.

@janden
Copy link
Collaborator

janden commented Jun 14, 2022

Ok sounds good then.

@janden janden merged commit b7d54e2 into kymatio:dev Jun 20, 2022
eickenberg pushed a commit that referenced this pull request Jul 5, 2022
* MAINT redefine cdgmm3d to call cdgmm in torch skcuda backend

* STY make more like torch_backend.py cdgmm3d implementation
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.

4 participants