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

Fix svd #10

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix svd #10

wants to merge 2 commits into from

Conversation

zhucaoxiang
Copy link
Collaborator

Somehow, the present code cannot normally execute SVD regularization. I checked the source code and made two changes:

  1. override Nlambda before current_potential related variables are declared;
  2. re-allocate RHS matrix with a dimension of ntheta_plasma*nzeta_plasma, rather than num_basis_function.

@landreman Please check if I did something incorrectly.

@zhucaoxiang zhucaoxiang requested a review from landreman August 9, 2018 00:20
Copy link
Owner

@landreman landreman left a comment

Choose a reason for hiding this comment

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

Yeah, it looks like I broke the svd scan on my May 10 commit "Extracted prepare_solve, solve, diagnostics to eliminate repeated code," since I missed the fact that RHS has a different size for the svd_scan compared to the usual solves.

Can I ask why you're using the SVD scan? As shown in the regcoil Nuclear Fusion paper, the SVD method seems pretty bad compared to standard regcoil.

regcoil_build_matrices.f90 seems like a surprising place to set nlambda = num_basis_functions-1. Could we do that in, say, regcoil_compute_lambda.f90 instead?

@zhucaoxiang
Copy link
Collaborator Author

zhucaoxiang commented Aug 9, 2018

Can I ask why you're using the SVD scan?

It's just my personal interest. I was wondering if I can find a numerically stable solution with minimum regularization. I understand that the current density regularization has been proven to be much better than the others, especially when we care about the maximum currents flowing in the coils. However, I was thinking the SVD regularization might be more suitable for my purpose. We can just leave the singular values that are large enough, say, abandon those lambda_i < 10^{-3} * lambda_max. So I just tested it and found SVD is not working.

Could we do that in, say, regcoil_compute_lambda.f90 instead?

There is no particular reason for me to put nlambda = num_basis_functions-1 in regcoil_build_matrices.f90. I found it was in regcoil_build_matrices.f90 that num_basis_functions is defined. I think any place after num_basis_functions is defined and before current_potential related terms are allocated is fine. You can definitely put them in regcoil_compute_lambda.f90 for consistence.

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

Successfully merging this pull request may close these issues.

2 participants