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

Mapping to/from rho tor norm #283

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

Conversation

AreWeDreaming
Copy link
Collaborator

Leaf of #258
Adds rho_tor_norm to the magnetic coordinates omas_physics.remap_flux_coordinates can map between. Wouldn't hurt if this someone double checked my math once #258 is merged. I did do a spot check and while the result wasn't perfect it was not too far off.
Arguably a unit test wouldn't hurt here, but I don't have time to write it at the moment.

Copy link

Stale pull request message

Copy link

Stale pull request message

@AreWeDreaming AreWeDreaming requested review from wdeshazer and removed request for wdeshazer January 6, 2025 19:51
Copy link
Collaborator

@torrinba torrinba left a comment

Choose a reason for hiding this comment

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

I see no new changes here, but if you can point me to where the rho_tor_norm conversion was added I'm happy to look it over

@AreWeDreaming
Copy link
Collaborator Author

def remap_flux_coordinates(ods, time_index, origin, destination, values):

and above

@torrinba
Copy link
Collaborator

torrinba commented Jan 8, 2025

Is this comment still accurate? It appears not to be...

def remap_flux_coordinates(ods, time_index, origin, destination, values):
    """
    Maps from one magnetic coordinate system to another. At the moment only supports
    psi <-> rho_pol

@torrinba
Copy link
Collaborator

torrinba commented Jan 8, 2025

Have you tested whether the transformations to/from phi are correct? If it isn't already in the ODS, I have some concerns about this working correctly, without accounting for the boundary terms

phi_spline = q_spline.antiderivative(1)

Of course the magnitude of phi doesn't matter if you're just interested in rho_tor_norm, but it could help avoid some of the concerns you've addressed with the wrong_sign_mask

@torrinba
Copy link
Collaborator

torrinba commented Jan 8, 2025

phi_grid = ods["equilibrium"]["time_slice"][time_index]["profiles_1d"]["phi"][psi_mask]

This line assumes that the data already exists, which might not be the case if the conversion is from rho_tor_norm. It would be more robust to protect against the ValueError in this case by falling back on add_phi_to_equilbrium_profiles_1d_ods as well

@torrinba
Copy link
Collaborator

torrinba commented Jan 8, 2025

phi_spl = InterpolatedUnivariateSpline(psi_grid[psi_grid_mask]

Assumes that psi is strictly increasing (AFAIK this is a requirement for InterpolatedUnivariateSpline), but the sign of psi depends on the COCOs so I don't think this is a safe assumption.

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