IMAS output core profiles coupling #1755
Conversation
MateoBell
force-pushed
the
imas_output_core_profiles_coupling
branch
from
0d02509 to
c2232a3
Compare
MateoBell
added
enhancement
| from torax._src.sources import source_profiles | ||
|
|
||
|
|
||
| def core_profiles_to_IMAS( |
There was a problem hiding this comment.
Would it be possible to break this function down into some smaller helper calls as the whole function is quite large. This is mainly to help with readability.
Maybe breaking down into a function each for:
- global quantities
- profiles
- main ions
- etc.
could help to structure this a bit. wdyt?
There was a problem hiding this comment.
yes completely ! I restructured it into coherent chunks. There is just some fields inside profiles_1d that I could not group.
Let me know if you think this looks better like this or if it still needs to be restructured 😄
|
Restructured to use helper calls with the following structure:
|
Nush395
left a comment
Nush395
left a comment
There was a problem hiding this comment.
Thanks for the change Mateo! Left some initial comments, hope they make sense!
| post_processing.PostProcessedOutputs | ||
| | list[post_processing.PostProcessedOutputs] | ||
| ), | ||
| core_profiles: state.CoreProfiles | list[state.CoreProfiles], |
There was a problem hiding this comment.
it feels a bit unnecessarily complex to accept both a list and a single item here. Which do you think is the more common use case? I'm wondering if we can just restrict this function only accept the Sequence rather than a single item.
I think that would:
- make the interface less confusing for users
- make the logic of the function simpler too
There was a problem hiding this comment.
I think the two main cases will be:
- Fill core_profiles at the end of a simulation with multiple time steps from a StateHistory object: then it will natively receive a list
- Fill a single time slice at the end of a step (inside the Muscle3 actor for example), so receive a single item
I agree it would be less confusing and logic lighter, but I didn't know what was the better solution to handle both cases, maybe we can just accept a Sequence, and therefore just assume we will provide directly a sequence containing a single item if we want to output a single timeslice ?
There was a problem hiding this comment.
if you think passing in a sequence of length 1 from the Muscle3 side is acceptable I think that would be a nicer solution.
| geometry: list[geometry.Geometry], | ||
| times: array_typing.FloatVector, | ||
| ) -> None: | ||
| ids.profiles_1d.resize(len(times)) |
There was a problem hiding this comment.
This creates the substructure of profiles_1d so we can access using profiles_1d[i]. Without doing this it will give an Index error: list index out of range
| cp_state.T_e.cell_plus_boundaries() * 1e3 | ||
| ) | ||
| ids.profiles_1d[i].electrons.density = cp_state.n_e.cell_plus_boundaries() | ||
| ids.profiles_1d[i].electrons.density_thermal = ( |
There was a problem hiding this comment.
what is the distinction between this and the above density field?
There was a problem hiding this comment.
From DD description (https://imas-data-dictionary.readthedocs.io/en/latest/generated/ids/core_profiles.html#ids-core_profiles):
density = density thermal + non_thermal (fast).
@jcitrin told me there is not fast particles yet in TORAX so I assume we can all consider them to be thermal, for now. Is it correct ?
There was a problem hiding this comment.
Correct, all thermal for now
| cp_state: state.CoreProfiles, | ||
| ) -> None: | ||
| def _fill_main_ion_quantities() -> None: | ||
| ion_properties = constants.ION_PROPERTIES_DICT[symbol] |
There was a problem hiding this comment.
where is this symbol defined?
| ids.profiles_1d[i].ion[index].label = symbol | ||
| ids.profiles_1d[i].ion[ | ||
| index | ||
| ].temperature = cp_state.T_i.cell_plus_boundaries() |
There was a problem hiding this comment.
this quantity was already computed for _fill_ion, is it necessary to extract and save the same info twice? Similar comment elsewhere
There was a problem hiding this comment.
I would say it's good to fill the temperature (or other quantity) for each individual ions even if it is the same. I think it can be useful for compatibility with other codes if you need to extract quantities for a specific ion to just go to its associated profiles_1d.ion substructure. Wdyt ?
| num_ions = num_of_main_ions + len(impurities) | ||
| ids.profiles_1d[i].ion.resize(num_ions) | ||
| # Fill main ions quantities | ||
| for ion, (symbol, frac) in enumerate(main_ion.items()): |
There was a problem hiding this comment.
where is this
symboldefined?
It is defined here in the for loop inside which the function is called. Is it okay to do it like this ?
3 participants
Add transfer function and tests to save TORAX profiles to an IMAS core_profiles or plasma_profiles IDS. Can receive inputs from a single or several time slices.
Inputs are chosen such that it should be straightforward to include a method into StateHistory using this function to save the IDS for an entire simulation in a similar way as it is done with
simulation_output_to_xr.@Nush395 : I kept the help function to compute the impurity density scaling and charge states of the individual densities on the cell_plus_boundaries grid. We can continue the discussion we had on whereas it is better to keep it like this or to extend the impurity quantities in
post_processed_outputsto cell_plus_boundaries grid.