-
Notifications
You must be signed in to change notification settings - Fork 9
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
Are the input for GRID, THEORY, SPHEREC, SPHEREC+, SPHEREM, SPHEREM+ degrees or radians? #5
Comments
Hi Xinglei, thanks for the issue and for going through the code. The input should be the degree specified in the readme and the experiments. You may have noticed a mistake here in the theory.py and grid_and_sphere.py implementations. I added these methods relatively short-term to the experiments, as the code to the sphere PEs just came out shortly before the original deadline. Another source of an error could be the cleanup of the code from the experiments before putting it here. In any case, it would be good to clarify and verify the consistency between the PEs. I am on parental leave and won't be able to run much code in the next week(s). Could you try adding the deg2rad conversion to theory.py and grid_and_sphere.py implementations and see if that makes a positive difference in the performances of these encoders? If that's the case, it would be good to amend the results and update the code base so that it is consistent. Thanks again for going through the code in such a detailed way and noticing this inconsistency. Marc |
Hi Marc, Thank you for your prompt response. I tested adding a line of code to the forward function of grid_and_sphere.py and run "grid+fcnet" and "grid+siren" on the land ocean classification task. The performance deteriorated a lot compared to using the degree input. Here I reported the results I got in my experiment.
I also noticed that the degree2rad operation are present in both Mai's and your code base. Maybe you accidentally commented this out and forgot to uncomment it? For your reference, I further tested sphere with fcnet and siren, under degree and rad values, and the results are as follows:
Please note these experiments used the default hyperparams. I am guessing that tuning the hyperparams for Rad values would result in better performance. (Please correct me if I am wrong!) To summarise, sphere* methods should use radian value rather than degree, whereas grid and theory should use degrees. There is no big problem with the results reported in the paper but you might need to bring the "deg2rad" operations back for those sphere methods in the codebase :) |
Hi @MarcCoru ,
Thank you for your excellent work! I really enjoyed reading it! I have a specific question that I hope you can enlighten me on.
I thought the input to all the positional encoders should be radian value (e.g., (-pi, pi)) rather than degrees (-180, 180), however, it seems that this is not the case for those in locationencoder/pe/grid_and_sphere.py.
Because, such a line of code "coords = torch.deg2rad(coords)" (wrap.py) is only found in direct.py, wrap.py and spherical_harmonics.py, but not in grid_and_sphere.py.
I am very confused. Do those methods proposed by Mai et al. really take degrees as input, or did I miss some important details? I know I might have referred to Mai et al.'s repo, but I find their codebase a bit unclear and they seem not very responsive.
Looking forward to your reply. Thank you.
The text was updated successfully, but these errors were encountered: