Parse line_id from MDSplus by AreWeDreaming · Pull Request #434 · gafusion/omas

AreWeDreaming · 2026-08-05T03:47:29Z

Description

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#94

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.2 → 0.94.3 for bug fixes, 0.94.2 → 0.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

smithsp · 2026-08-05T07:08:13Z

+def roman_to_int(roman):
+    total = 0
+    highest = 0
+    for char in reversed(roman):
+        value = ROMAN_DIGITS[char]
+        total += value if value >= highest else -value
+        highest = max(highest, value)
+    return total


Is there not a library that implements this?

I recommend adding a few tests for this function.

Do tests that happen inside imas_composer suffice?

If they test this, that would be sufficient.

They do now. https://github.com/GA-FDP/imas_composer/blob/4e730397e2b9fa6fd49d57a9a98b5cb2ed908a24/tests/test_charge_exchange_lineid.py#L67

Co-authored-by: Torrin <1843540+torrinba@users.noreply.github.com>

torrinba · 2026-08-06T00:36:28Z

+        raise ValueError(f'{sub} channel {channel:02d}: cannot parse LINEID {lineid!r}')
+    z_n, a = CER_ELEMENTS[element]
+    z_ion = roman_to_int(roman)
+    symbol = 'H' if element == 'D' else element


The IMAS DD specifically documents that the symbol "D" is allowed. So it doesn't seem like there is a need to change to "H" here.

IMAS also suggests the labels should be "D+", "He+2", "C+6", etc. so it seems like we should try to follow that syntax unless there are problems with it (in which case we should open an issue - this field has been changed to "name" in IMAS 4 so that would be worth keeping in mind there).

I would defer fixing this for now but open an issue to tackle this later. The reason for this is that we need to also change it in IDA.

Why is the label essential in IDA? It seems like z_n and z_ion are all you need to filter the ions (and a if you care about isotopes)

That's not how it is implemented. If the labeling is consistent this is easier: https://github.com/GA-IDA/IDA/blob/2d1b056d9a24fb1262d5b56a69616d3c56d5b251/config/models/charge_exchange.yaml#L19

If the label is only used in IDA to distinguish the quantity being measured then it doesn't seem like the format should make any difference. It would only matter if you're parsing the label to extract the ion charge or mass (or have it hard coded anywhere like it is in the config)

Except that right now in IDA this specific label style is also used for the underlying parametrization.

Sure, but the names of the IDA parametrizations don't actually have to match the ion labels and could be selected based on z_n, z_ion, and a

We should try to be consistent

Co-authored-by: Torrin <1843540+torrinba@users.noreply.github.com>

smithsp · 2026-08-06T20:01:12Z

+def roman_to_int(roman):
+    total = 0
+    highest = 0
+    for char in reversed(roman):
+        value = ROMAN_DIGITS[char]
+        total += value if value >= highest else -value
+        highest = max(highest, value)
+    return total


If they test this, that would be sufficient.

smithsp · 2026-08-06T20:02:05Z

So all of this got migrated to its own CER omas fetching?

I don't think everything got migrated but I bet that we can find a similar and probably better maintained script somewhere in OMFIT. This script relies on OMFIT in OMAS which is something we are trying to avoid.

AreWeDreaming · 2026-08-25T20:52:17Z

@torrinba can this be merged? It seems like the sister branch in imas_composer was merged which now causes trouble because imas_composer relies on an omas piece provided by this branch for a test the absence of which breaking test discovery

torrinba

Yeah, this should be fine to merge with the understanding that the ion names are going to change again soon. IMAS will be updating their documentation to recommend the atomic mass, at least for the less common isotopes, with the exception of hydrogen. We don't have to wait for them, but I don't think it needs to be the highest priority yet either

smithsp · 2026-08-27T06:34:27Z

Suspicious that the tests for python 3.7-3.10 pass, but 3.11-3.14 fail.

torrinba · 2026-08-27T06:47:30Z

Suspicious that the tests for python 3.7-3.10 pass, but 3.11-3.14 fail.

It has been ignored for a while. Probably related to matplotlib changes
#418 (comment)

Fix bug for Helium beams

…e_id

Parse line_id from MDSplus

d6f9158

AreWeDreaming requested a review from torrinba August 5, 2026 03:47

smithsp reviewed Aug 5, 2026

View reviewed changes

Remove dead _d3d_cer (OMFIT copy-pasta)

efdf917

torrinba reviewed Aug 5, 2026

View reviewed changes

Comment thread omas/machine_mappings/d3d.py Resolved

Update omas/machine_mappings/d3d.py

3181c0b

Co-authored-by: Torrin <1843540+torrinba@users.noreply.github.com>

torrinba reviewed Aug 6, 2026

View reviewed changes

Comment thread omas/machine_mappings/d3d.py Resolved

Update omas/machine_mappings/d3d.py

da86959

Co-authored-by: Torrin <1843540+torrinba@users.noreply.github.com>

AreWeDreaming requested a review from smithsp August 6, 2026 17:32

smithsp approved these changes Aug 6, 2026

View reviewed changes

torrinba approved these changes Aug 25, 2026

View reviewed changes

AreWeDreaming and others added 5 commits September 14, 2026 12:05

Fix bug for Helium beams

0614673

Merge branch 'ssd/process_cer_line_id' into ssd/fix_d3d_He_beam_mapping

eb4ce32

Align assumptions on GAS values with reality

eb274bd

Merge pull request #439 from gafusion/ssd/fix_d3d_He_beam_mapping

e14f87e

Fix bug for Helium beams

Merge remote-tracking branch 'origin/master' into ssd/process_cer_lin…

25730be

…e_id

torrinba merged commit 5490ffd into master Oct 8, 2026
8 checks passed

torrinba deleted the ssd/process_cer_line_id branch October 8, 2026 18:09

Conversation

AreWeDreaming commented Aug 5, 2026

Description

Type of Change

Testing

Pre-Merge Checklist

Additional Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

torrinba Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torrinba Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torrinba Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AreWeDreaming commented Aug 25, 2026

Uh oh!

torrinba left a comment

Choose a reason for hiding this comment

Uh oh!

smithsp commented Aug 27, 2026

Uh oh!

torrinba commented Aug 27, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

torrinba Aug 6, 2026 •

edited

Loading

torrinba Aug 6, 2026 •

edited

Loading

torrinba Aug 6, 2026 •

edited

Loading