(torax): Issue#1358 Add dynamic variable access to PlotData class
#1874
Conversation
|
@jcitrin , take a look !! |
jcitrin
left a comment
jcitrin
left a comment
There was a problem hiding this comment.
My one main comment is that we can relax a bit the requirement for backwards compatibility in plotruns_lib. What needs to be backwards compatible is only the configs in plotting/configs. If plotruns_lib_test passes, then we should be OK. The specific in-line comments are all in this direction:
| and any variable available in the output file through dynamic attribute access. | ||
| Attributes: | ||
| Hardcoded Attributes (for backward compatibility): |
There was a problem hiding this comment.
You can remove the hard-coded attributes in PlotData. Just make as properties any currently hard-coded attributes that do not directly exist in the child DataTrees. Looking at the list, I think it's just P_sink .
|
|
||
| return PlotData( | ||
| profiles_dataset=profiles_dataset, | ||
| scalars_dataset=scalars_dataset, |
There was a problem hiding this comment.
In the hard-coded attributes defined below, I noticed a bug with 3. As a drive-by, I noticed a bug with Ip_profile. It is currently defined to be identical to scalars.Ip, and therefore Ip_profile is a misnomer. We also have a profiles.Ip_profile and I wonder how they now interact. I would check this closely. For backwards incompatibility I would indeed override profiles.Ip_profile with this buggy definition of Ip_profile as we have it now, but please add a TODO to change this in V2.
i.e. it's not a profile, it's a scalar. For now I suggest making Ip
| profiles_dataset: xr.Dataset | None = None, | ||
| scalars_dataset: xr.Dataset | None = None, |
There was a problem hiding this comment.
no need for optional. You can require that the components of the dataset are input
| scalars_dataset: xr.Dataset | None = None, | ||
| dataset: xr.Dataset | None = None, | ||
| numerics_dataset: xr.Dataset | None = None, | ||
| **kwargs, |
There was a problem hiding this comment.
as discussed can just remove this
| f"Variable '{name}' not found in output file datasets." | ||
| ) | ||
|
|
||
| def __dataclass_fields__(self): |
There was a problem hiding this comment.
Now that we're going to remove all the hard-coded attributes, you don't need to override dataclass_fields here. Instead, you can make a function that flattens the attributes in the components of PlotData and the properties, and returns a flat dict. Then, assign that dict to plotdata_attrs in plot_run and remove any call to __dataclass_fields__
3 participants
Description:
Enables
PlotDatato dynamically access any variable from output datasets, not just hardcoded attributes.Previously,
PlotDataonly supported predefined attributes (T_i, T_e, n_e, etc.). Users couldn't plot custom diagnostic variables from output files without modifying the class. This PR solves this issue.Issue Solved #1358