Repository navigation
Equilibrium - BUGFIX - Select the rectangular profiles_2d by grid_type in read_imas - #476
Conversation
4b95561 to
c57b7c8
Compare
jhalpern30
left a comment
There was a problem hiding this comment.
Nice work! Some requested changes before we get this merged, mainly focusing on comments. I challenge you to not use Claude to make the below changes on this PR :)
| "Ensure the 2D ψ(R,Z) map is stored in dd.equilibrium.") | ||
| end | ||
| prof2d = eqt.profiles_2d[1] | ||
| # Solvers may store several profiles_2d representations of the same equilibrium. |
There was a problem hiding this comment.
There is no need to write an entire paragraph as a comment here (and above). Think - what is the most useful (if any) pieces of information a human should know about this code? LLMs have a tendency to flood code comments with extraneous information we've been trying to avoid, so its good practice to read through everything you propose to add and write it from a human perspective. My general rule of thumb is if you're adding more lines of comment than code (as is the case in this PR), you need to take a look again.
For example: a good comment here might be "find 2d profiles on the rectangular grid for a general input equilibrium"
| # returning missing, so a lone 2D map is taken as-is: that case is unambiguous and | ||
| # keeps equilibria that never filled grid_type working exactly as before. | ||
| grid_index(p) = getproperty(p.grid_type, :index, 0) | ||
| prof2d = if length(eqt.profiles_2d) == 1 |
There was a problem hiding this comment.
You say this section is exactly how FUSE locates the grid - can you point me to the relevant source code here?
There was a problem hiding this comment.
Yes, to what I understand FUSE selects the grid by type and not position. And the source code for this is src/actors/equilibrium/equilibrium_actor.jl lines 164 and 455, fresco_actor.jl line 85 and eggo_actor.jl line 132.( See equilibrium_actor.jl#L164, #L455, fresco_actor.jl#L85 and eggo_actor.jl#L132. So Claude suggested to use grid_type.index == 1 (the same identifier, since 1 => :rectangular in IMASdd) rather than that helper, because findfirst(:rectangular, …) skips entries whose grid_type is unset.
There was a problem hiding this comment.
Looking at all of these FUSE functions, it looks like they use the IMASdd helper
eqt2d = findfirst(:rectangular, eqt.profiles_2d)is there a reason you aren't using this? It seems to be more convenient/clear.
There was a problem hiding this comment.
Yes it was for the mock to not fail, but we can solve this within the tests so switched to findfirst(:rectangular, eqt.profiles_2d). I added one line to the test mock (prof2d.grid_type.index = 1), because the helper skips maps without a grid type and the mock was the only input that didn't set one. Real files (EFIT.jl, FRESCO, CHEASE, TEQUILA) all set it.
60b8438 to
bb3340e
Compare
bb3340e to
67b48ea
Compare
jhalpern30
left a comment
There was a problem hiding this comment.
Everything looks good now, thanks @calin1989! I will merge this in once tests finish
Release note
GPEC can now read IMAS equilibria that store more than one 2D map, such as those from TEQUILA.
read_imaspreviously always took the firstprofiles_2dentry, which for TEQUILA is not the rectangular R,Z grid.Regression report
Notes for reviewers
read_imasnow selects the rectangularprofiles_2dentry with IMASdd'sfindfirst(:rectangular, …), as FUSE does, and stops with an error if there is none. The test mock now setsgrid_type.index = 1, as real IMAS writers do.🤖 Generated with Claude Code