Skip to content

Conversation

@MuawizChaudhary
Copy link
Collaborator

Back in the good old days, before I existed, Kymatio reimplemented various calls that a nn.module had. These included .cpu,cuda, and were often implemented using a _apply call.

A reviewer of our JMLR submission kindly noted that we could just inherit from a nn.module instead of reimplementing all those calls.

This _apply_filters tucked and hidden away in the 3d utils code is the last remaining vestige of this old way of doing things.

@codecov-commenter
Copy link

Codecov Report

Merging #830 (4bcb6fa) into dev (ca9073f) will increase coverage by 0.10%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev     #830      +/-   ##
==========================================
+ Coverage   88.03%   88.14%   +0.10%     
==========================================
  Files          64       64              
  Lines        2323     2319       -4     
==========================================
- Hits         2045     2044       -1     
+ Misses        278      275       -3     
Flag Coverage Δ
jenkins_main 88.14% <ø> (+0.10%) ⬆️

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

Impacted Files Coverage Δ
kymatio/scattering3d/utils.py 96.55% <ø> (+8.67%) ⬆️

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 ca9073f...4bcb6fa. Read the comment docs.

@lostanlen
Copy link
Collaborator

Can't use 'tar -xzf' extract archive file: /home/runner/work/_actions/_temp_735093b6-e20a-47f8-9c61-589d4c653a4c/fb47e774-6995-4eb3-8ca0-770e1e7792dc.tar.gz. return code: 2.

This build-pip CI looks very brittle

@edouardoyallon
Copy link
Collaborator

Be extra careful. The reason it was done this way is because we wanted an access to the filters via psi[j] and that the filters were stored as a parameter. Unfortunatly, when using .cuda(), the pointer was not going through the correct direction.

@MuawizChaudhary
Copy link
Collaborator Author

Be extra careful. The reason it was done this way is because we wanted an access to the filters via psi[j] and that the filters were stored as a parameter. Unfortunatly, when using .cuda(), the pointer was not going through the correct direction.

hm, wasnt this because we weren't inheriting nn.module?

A git grep for "_apply_filters" shows that this function is not used anywhere.

@edouardoyallon
Copy link
Collaborator

no clue, i'm just saying it was there for this reason

@MuawizChaudhary
Copy link
Collaborator Author

Can we merge?

@lostanlen lostanlen merged commit 2d5abae into kymatio:dev Jun 13, 2022
eickenberg pushed a commit that referenced this pull request Jul 5, 2022
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