Conversation
torrinba
left a comment
There was a problem hiding this comment.
I fully support adding the new field that includes systematic and statistical error separately. This is probably an oversight that should be addressed in most diagnostic IDS fields.
I'm still a bit nervous about removing data_error_upper though. I'm not sure whether that is used anywhere and it isn't very easy to track since it could be part of a ufloat variable. I'm not sure whether there is an analogous way of handling uncertainties in Julia, but FUSE does use the Interferometer mappings. I don't see anywhere that directly uses the uncertainty, but I haven't investigated the plots closely or have enough familiarity be confident that removing this won't affect something.
If @jmcclena confirms that this won't affect FUSE I'm happy to let the merge go forward. Otherwise, it would be safer to keep data_error_upper in addition to the new error field so that this change doesn't risk breaking anything. The would potentially duplicate some data, unless you wanted to modify error to only include one component, but neither seems like as much of a concern.
|
@torrinba I mostly removed it to get your input on how you feel about it. After having immersed myself in IMAS (the IO flavor in particular) for a week, I feel less strongly about removing the legacy field. I have readded it as the sum of std. dev. and sys. error. |
jmcclena
left a comment
There was a problem hiding this comment.
Looks fine to me as you're not deleting the original imas schema.
Description
This changes how the uncertainty is defined for interferometry is defined.
Now the machine mappings use
inteferometer.channel.ne_line.errorwhich is of size (2, N_time). Index 0 is for the statistical error and Index 1 is for the systematic uncertainty.At the moment this is breaking because the old field
interferometer.channel.ne_line.data_error_upperhas been removed. If that's a bad idea I can add it back in but the systematic error and the statistical error can be about the order of magnitude so the distinction between them here is important.Type of Change
Testing
Tested against IMAS composer GA-FDP/imas_composer#24
Pre-Merge Checklist
omas/versionfile0.94.2→0.94.3for bug fixes,0.94.2→0.95.0for new features)Additional Notes