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

LPD biasses do not follow the original paper #139

Open
D1rk123 opened this issue Oct 16, 2024 · 3 comments
Open

LPD biasses do not follow the original paper #139

D1rk123 opened this issue Oct 16, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@D1rk123
Copy link
Contributor

D1rk123 commented Oct 16, 2024

In the LPD implementation the bias terms of the convolution layers are turned off (bias=False in nn.Conv2d). However, in the original LPD paper they say that the convolution layers do have biasses. (Section IV.A In the equation after the line "where the k:th component is given by"). I also checked the original code, and it does not specify the bias, but the Tensorflow default is True.

Was it a deliberate choice to not follow the paper? Otherwise I think it's better to change the code so that bias=True for all convolution layers.

@AnderBiguri
Copy link
Member

Totally agree!

I think this is caused by LION still being a bit WIP and sometimes stuff gets changed because I test this or that paper etc.
Admittedly the current LPD is the one I found that works better than the boilerplate one, but recently we indeed decided that all default settings should follow the original paper, so we should 100% change this, and make the current default be something like LION_default_parameters (or possibly a neater name) like we have for the alternatives:

@staticmethod
def default_parameters():
LPD_params = LIONParameter()
LPD_params.n_iters = 10
LPD_params.data_channels = [7, 32, 32, 5]
LPD_params.reg_channels = [6, 32, 32, 5]
LPD_params.learned_step = False
LPD_params.step_size = None
LPD_params.step_positive = False
LPD_params.mode = "ct"
LPD_params.instance_norm = False
return LPD_params
@staticmethod
def continous_LPD_paper():
LPD_params = LIONParameter()
LPD_params.n_iters = 5
LPD_params.data_channels = [7, 32, 32, 32, 5]
LPD_params.reg_channels = [6, 32, 32, 32, 5]
LPD_params.learned_step = True
LPD_params.step_size = None
LPD_params.step_positive = True
LPD_params.mode = "ct"
LPD_params.instance_norm = True
return LPD_params

@AnderBiguri AnderBiguri added the bug Something isn't working label Oct 16, 2024
D1rk123 added a commit to D1rk123/LION that referenced this issue Oct 16, 2024
AnderBiguri added a commit that referenced this issue Oct 16, 2024
Add conv_bias parameter in LPD (Issue #139)
@D1rk123
Copy link
Contributor Author

D1rk123 commented Oct 17, 2024

Thanks for your quick reply!

Having a separate original paper settings, and LION default settings sounds like a good idea to me. In my (somewhat limited) experiments, LPD also performed better without convolution biasses. In the pull request I didn't include the two versions of the settings yet.

@AnderBiguri
Copy link
Member

AnderBiguri commented Oct 17, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants