Skip to content

Interferometry systematic error#401

Open
AreWeDreaming wants to merge 7 commits intomasterfrom
systematic_error_for_int
Open

Interferometry systematic error#401
AreWeDreaming wants to merge 7 commits intomasterfrom
systematic_error_for_int

Conversation

@AreWeDreaming
Copy link
Collaborator

@AreWeDreaming AreWeDreaming commented Mar 16, 2026

Description

This changes how the uncertainty is defined for interferometry is defined.
Now the machine mappings use inteferometer.channel.ne_line.error which 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_upper has 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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Code refactoring
  • Other (please describe):

Testing

Tested against IMAS composer GA-FDP/imas_composer#24

Pre-Merge Checklist

  • OMFIT has been tested with this OMAS version and you will create a PR on OMFIT that updates the OMAS submodule once this has been merged.
  • Version number has been incremented if this is a major modification or important bug fix
    • To increment the version: Edit the version number in omas/version file
    • Follow semantic versioning: MAJOR.MINOR.PATCH (e.g., 0.94.20.94.3 for bug fixes, 0.94.20.95.0 for new features)
    • A GitHub release will be automatically created when this PR is merged if the version number has changed

Additional Notes

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 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.

@AreWeDreaming
Copy link
Collaborator Author

@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.

Copy link
Collaborator

@jmcclena jmcclena left a comment

Choose a reason for hiding this comment

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

Looks fine to me as you're not deleting the original imas schema.

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.

3 participants