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.

Sign up for GitHub

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

Jump to bottom

FIX Downsampling Bugs #62

Merged
merged 9 commits into from
Dec 2, 2024
Merged

FIX Downsampling Bugs #62

merged 9 commits into from
Dec 2, 2024

Conversation

xir4n
Copy link
Collaborator

@xir4n xir4n commented Nov 22, 2024

Changed default arguments in DTCWT:

  1. Set alternate_gh=False as the default value for dtcwt
  2. Resolved an issue where the normalization during downsampling was incorrect.
  3. Enhanced to_conv1d method
  4. Adjusted the downsampling level to optimize performance and accuracy.

@xir4n xir4n requested a review from lostanlen November 24, 2024 12:11
lostanlen
lostanlen reviewed Nov 24, 2024
View reviewed changes
tests/test_nn.py Outdated Show resolved Hide resolved
tests/test_nn.py Outdated Show resolved Hide resolved
tests/test_dtcwt.py Show resolved Hide resolved
tests/test_dtcwt.py Outdated Show resolved Hide resolved
tests/test_dtcwt.py Outdated Show resolved Hide resolved
murenn/dtcwt/nn.py Outdated Show resolved Hide resolved
murenn/dtcwt/nn.py Outdated Show resolved Hide resolved
murenn/dtcwt/nn.py Show resolved Hide resolved
murenn/dtcwt/nn.py
@@ -179,13 +191,13 @@ def __init__(self, J_phi):
J=1,
level1="near_sym_b",
skip_hps=True,
padding_mode="zeros",
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure which padding mode is better, what I'm trying to do is simply use the default padding mode for all cases when applying dtcwt

@lostanlen
Copy link
Contributor

Thank you. Note that this PR makes an important breaking change to the MuReNN API: alternate_gh=False by default

@xir4n xir4n requested a review from lostanlen November 29, 2024 17:52
lostanlen
lostanlen reviewed Nov 29, 2024
View reviewed changes
tests/test_nn.py Outdated Show resolved Hide resolved
murenn/dtcwt/nn.py Show resolved Hide resolved
murenn/dtcwt/nn.py Show resolved Hide resolved
murenn/dtcwt/nn.py Show resolved Hide resolved
murenn/dtcwt/transform1d.py Show resolved Hide resolved
tests/test_nn.py Show resolved Hide resolved
@lostanlen
Copy link
Contributor

Hello @xir4n , i have provided some more feedback. Please click the "Resolve conversation" to mark my comments as resolved. The two remaining things are:

  • this diagram, which i don't think is correct
           conv1d        IDTCWT
        δ --------> w_jq -------> y_jq
  • whether defaulting to alternate_gh=False is still necessary

Thanks!

@xir4n xir4n mentioned this pull request Dec 2, 2024
Closed
@lostanlen lostanlen merged commit 986579d into main Dec 2, 2024
6 checks passed
This was referenced Dec 2, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lostanlen lostanlen lostanlen left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xir4n @lostanlen