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

Implement simplified surfaces and faster contouring #217

Merged
merged 15 commits into from
Dec 4, 2024
Merged

Conversation

bclyons12
Copy link
Member

This creates a simplified, lighter-weight version of flux surfaces suited to some applications (e.g., FRESCO). It also uses the new IMASutils contouring where applicable.

@bclyons12 bclyons12 added the enhancement New feature or request label Nov 20, 2024
@bclyons12 bclyons12 requested a review from orso82 November 20, 2024 06:17
@bclyons12 bclyons12 self-assigned this Nov 20, 2024
@bclyons12
Copy link
Member Author

@orso82 I didn't modify flux_surface() since that has lots of fancy logic for open surfaces that I can't guarantee would be covered by IMASutil's contouring. It looks like we could do the same thing for trace_surfaces() that I did for trace_simple_surfaces(), and for the other version of find_psi_boundary() like I did for my version.

@bclyons12
Copy link
Member Author

Also, once this PR is deemed ready to merge, this is a circumstance where we should run all the FUSE tests on it first. That will stress test the new contouring routines.

@orso82
Copy link
Member

orso82 commented Nov 21, 2024

To run the FUSE tests using your changes, just open a PR in FUSE on a branch with the same simple_surface name that you have used here.

@bclyons12 bclyons12 changed the title Implement simplifeid surfaces and faster contouring Implement simplified surfaces and faster contouring Nov 21, 2024
@bclyons12
Copy link
Member Author

The IMAS 2.0 refactoring has been merged into this, which means both versions of find_psi_boundary() now use the new contouring.

@bclyons12
Copy link
Member Author

@orso82 trace_surfaces() now uses the new IMASutils contouring as well. Only flux_surface (and other fucntions that depend on that) is left untouched because of the other types of surfaces that it can contour.

@bclyons12
Copy link
Member Author

@orso82 All the FUSE regressions pass (ProjectTorreyPines/FUSE.jl#770) so I'll merge this unless you want to make the find_psi_boundary more robust under this PR.

@orso82
Copy link
Member

orso82 commented Nov 23, 2024

@bclyons12 this is great, although it's unclear to me what's the difference between trace_surfaces and trace_simple_surfaces.

Do we really need both?

If we can't just have a single version, could you add a description to the docstring oftrace_simple_surfaces and perhaps highlight the difference with respect to trace_surfaces

@bclyons12
Copy link
Member Author

@orso82 I cleaned up trace_simple_surfaces even more and added some documentation. The idea is that the FluxSurface struct has a lot of information, but if all we want is to perform flux surface averaging, we can get away with a lot less. In FRESCO where I might need to trace surfaces every iteration, this reduces the computation needed.

ig, jg = pts[argmax(pts_in_wall)] # this are the indices for the guess inside the wall
rguess, zguess = r[ig], z[jg]
RA, ZA = find_magnetic_axis(r, z, PSI_interpolant, psi_sign; rguess, zguess)
if !isempty(wall_r)
Copy link
Member

@orso82 orso82 Dec 3, 2024

Choose a reason for hiding this comment

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

Thank you for doing this @bclyons12 ! 👍

I wonder if we should add this logic directly to the find_magnetic_axis() function.

Copy link
Member Author

Choose a reason for hiding this comment

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

You would have to pass the wall to it then, so up to you. You could make a separate function that takes the wall as an argument and does this I suppose.

@bclyons12
Copy link
Member Author

@orso82 If the FUSE tests pass (ProjectTorreyPines/FUSE.jl#770), I would like to merge this.

@bclyons12
Copy link
Member Author

All FUSE tests pass (ProjectTorreyPines/FUSE.jl#770)

@bclyons12 bclyons12 merged commit a634cd9 into master Dec 4, 2024
2 checks passed
@orso82 orso82 deleted the simple_surface branch February 11, 2025 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants