Make Q2 customizable - #854
Merged
Merged
Conversation
Collaborator
lostanlen
reviewed
Jun 8, 2022
lostanlen
left a comment
lostanlen
left a comment
Collaborator
There was a problem hiding this comment.
nice work, have you tested it?
The code looks greatl but the documentation could be improved a bit. See my comments
Thanks!
cyrusasfa
requested changes
Jun 8, 2022
changhongw
commented
Jun 9, 2022
changhongw
commented
Jun 9, 2022
MuawizChaudhary
requested changes
Jun 9, 2022
lostanlen
reviewed
Jun 9, 2022
Collaborator
|
i have attempted a rebase. fingers crossed |
Collaborator
yes the merge with your latest changes on dev should be sufficient |
MuawizChaudhary
approved these changes
Jun 9, 2022
MuawizChaudhary
left a comment
MuawizChaudhary
left a comment
Collaborator
There was a problem hiding this comment.
This PR adds a feature where users can specify the Q parameter of the 2nd order wavelets.
I approve this change.
Collaborator
|
we're waiting for @cyrusvahidi to approve the tests |
cyrusasfa
reviewed
Jun 12, 2022
cyrusasfa
reviewed
Jun 12, 2022
cyrusasfa
requested changes
Jun 14, 2022
Collaborator
Author
|
|
MuawizChaudhary
approved these changes
Jun 16, 2022
MuawizChaudhary
approved these changes
Jun 16, 2022
lostanlen
reviewed
Jun 16, 2022
lostanlen
left a comment
lostanlen
left a comment
Collaborator
There was a problem hiding this comment.
Looks like tests are passing again. It's just that the current error message is not accurate:
Q should be an integer, a 1-tuple, or a 2-tuple.
Collaborator
if not A.is_cuda or not B.is_cuda:
> raise TypeError('Input and filter must be CUDA tensors.')
E TypeError: Input and filter must be CUDA tensors. |
added 2 commits
June 17, 2022 09:34
cyrusasfa
approved these changes
Jun 17, 2022
Collaborator
|
Thank you so much @changhongw 🎉 |
lostanlen
approved these changes
Jun 17, 2022
This was referenced Jun 17, 2022
Closed
lostanlen
added a commit
to danedane-haider/kymatio
that referenced
this pull request
Jun 19, 2022
in light of kymatio#854 and kymatio#882 enable TeX rendering
lostanlen
added a commit
that referenced
this pull request
Jun 20, 2022
by Daniel Haider and Vincent Lostanlen approved by Muawiz Chaudhary and Joakim Andén * Update plot_filters.py * Update plot_filters.py * Update plot_filters.py Edits as required: - original formatting - line width reduction - psi <- psi_i * Update plot_filters.py * update examples/1d/plot_filters in light of #854 and #882 enable TeX rendering Co-authored-by: Vincent Lostanlen <vincent.lostanlen@ls2n.fr>
eickenberg
pushed a commit
that referenced
this pull request
Jul 5, 2022
* move Q check to ScatteringBase1D.build * Tuneable Q2 * test on GPU * Update base_frontend.py * Update base_frontend.py * Update test_torch_scattering1d.py * skip `_skcuda` * Update test_torch_scattering1d.py Co-authored-by: Vincent Lostanlen <vincent.lostanlen@ls2n.fr>
eickenberg
pushed a commit
that referenced
this pull request
Jul 5, 2022
by Daniel Haider and Vincent Lostanlen approved by Muawiz Chaudhary and Joakim Andén * Update plot_filters.py * Update plot_filters.py * Update plot_filters.py Edits as required: - original formatting - line width reduction - psi <- psi_i * Update plot_filters.py * update examples/1d/plot_filters in light of #854 and #882 enable TeX rendering Co-authored-by: Vincent Lostanlen <vincent.lostanlen@ls2n.fr>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #853
This PR makes it flexible for users to specify the number of wavelets per octave at the first and second order scattering. Both cases are working:
Qas an integer:Q = Q1. In this case, we useQ1wavelets per octave at the first order and that for the second order defaults to one.Qas a tuple:Q = (Q1, Q2), where users can specify bothQ1andQ2according to their needs.