Skip to content

Patching small issue with dr_sep for some equilibria#49

Merged
georgeholt1 merged 3 commits into
mainfrom
dr_sep_patch
Mar 31, 2026
Merged

Patching small issue with dr_sep for some equilibria#49
georgeholt1 merged 3 commits into
mainfrom
dr_sep_patch

Conversation

@kpentland
Copy link
Copy Markdown
Collaborator

Just patching a small issue that returned an incorrect value for dr_sep for some "weird" shaped equilibria. Should work fine for most but this should now work for all.

Main changes:

  • instead of filtering the x-points by those that have psi values closest to psi_boundary, we instead keep only those inside the tokamak wall, then choose the two closest to the magnetic x-point.
  • the values of psi at these two x-points are then used to find dr_sep.

@kpentland kpentland added the enhancement New feature or request label Oct 10, 2025
@georgeholt1
Copy link
Copy Markdown
Contributor

Thanks @kpentland! This method by filtering based on locations inside the wall is definitely an improvement. Couple points to discuss though:

  1. The old way ordered the X-points lower-to-upper. In the docstring, it still says: "By convention, the function assumes the first X-point corresponds to the lower X-point, and the second to the upper X-point.". With this change, this is no longer necessarily the case. I can't think of a reason why we would need to maintain this convention, so my suggestion is to either fix the docstring or do a sort on the X-points:

    closest_xpts_sorted = closest_xpts_sorted[np.argsort(closest_xpts_sorted[:, 1])]
  2. _updatePlasmaPsi uses a local opt variable and only sets self.psi_axis = opt[0][2]. The new code references self.opt[0, 0:2], but this could potentially become not up-to-date. I suggest either using self.magneticAxis()[0:2], or update _updatePlasmaPsi to that self.opt is reliably set.

  3. Obviously calling this function for single-null plasmas doesn't make sense, but it might still happen if people are using it in an automated workflow. Therefore, it would be an improvement to have a warning so that the downstream closest_xpts_sorted[1, 2] doesn't raise an index error:

    if xpts_inside_wall.shape[0] < 2:
        warnings.warn("Fewer than 2 X-points found inside the wall; cannot compute dr_sep.")
        return [None, None]
  4. # get indices of the two cloest to magnetic axis
    cloest --> closest

@kpentland
Copy link
Copy Markdown
Collaborator Author

Hi @georgeholt1,

Thanks for this!

I've made the changes so that the code reflects the docstrings and purposely uses the lower X-point first (I think this is a typical convention and if different for other tokamaks, people can flip the signs). It also now throws an error if less than two X-points are found within the first wall. I've also called the eq.magneticAxis() function to make things more stable.

@georgeholt1 georgeholt1 merged commit 972293b into main Mar 31, 2026
1 check passed
@georgeholt1
Copy link
Copy Markdown
Contributor

Thanks again @kpentland

@georgeholt1 georgeholt1 deleted the dr_sep_patch branch March 31, 2026 16:33
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