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

Question regarding kzetafinder_equation #22

Closed
Edenhofer opened this issue Feb 16, 2024 · 3 comments
Closed

Question regarding kzetafinder_equation #22

Edenhofer opened this issue Feb 16, 2024 · 3 comments
Assignees
Labels
question Further information is requested

Comments

@Edenhofer
Copy link

#x = π ./ (1 .+ kzeta)
#return sin.(x) ./ x .- finder.ζ0
x = 1 ./ (1 .+ kzeta)
return sinc.(x) .- finder.ζ0

The referenced formula (Equation 47 of Psaltis et al. 2018) contains an additional kzeta in the argument to sinus. Is that potentially missing here?

Part of openjournals/joss-reviews#6354 .

@Edenhofer
Copy link
Author

A related question about kzetafinder_equation in src/kzetafinders/dipole.jl and src/kzetafinders/vonmises.jl , shouldn't the square difference be penalized instead of value - A^2?

@kazuakiyama kazuakiyama added the question Further information is requested label Feb 29, 2024
@kazuakiyama
Copy link
Member

kazuakiyama commented Oct 16, 2024

We are very sorry that our response came very late. @annatartaglia had a career transition after we received the report and unfortunately, it took a long time for us to come back to work on the revision. The package revision is almost done and here we would like to answer your questions.

The referenced formula (Equation 47 of Psaltis et al. 2018) contains an additional kzeta in the argument to sinus. Is that potentially missing here?

Thanks for carefully going through the equation. Indeed, this implementation is slightly different from Equation 47 of Psaltis et al. 2018. To our knowledge, Equation 47 has a typo --- this extra kzeta you found is not required. Unfortunately, for some reason, the paper didn't complete the review process or have another revision, and the typo has been left in the arxiv posting for a while.

You can find the correct model implemented in the current community standard package (e.g. see l.106 of this code in eht-imaging library). This is a reference implementation of Psaltis et al. 2018. We added the clarification in the source code (commit: a619d55).

A related question about kzetafinder_equation in src/kzetafinders/dipole.jl and src/kzetafinders/vonmises.jl , shouldn't the square difference be penalized instead of value - A^2?

This came from the definition of NonlinearProblem in NonlinearSolve.jl used in the package. NonlinearProblem needs the actual equation to be solved rather than the cost function (e.g. squared differences) for the optimization. We tried other optimization packages available in Julia (e.g. Optim.jl and Optimization.jl), and we found NonlinearSolve was the fastest among the packages we tried. This leads to the current implementation choice and definition of the equation.

Please let us know if you have any additional questions. Again, we thank you for your thorough reading of the library.

@Edenhofer
Copy link
Author

Thank you for your detailed reply! This all makes sense :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants