Parse line_id from MDSplus - #434
Conversation
| 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 |
There was a problem hiding this comment.
Is there not a library that implements this?
I recommend adding a few tests for this function.
There was a problem hiding this comment.
Do tests that happen inside imas_composer suffice?
There was a problem hiding this comment.
If they test this, that would be sufficient.
There was a problem hiding this comment.
Co-authored-by: Torrin <1843540+torrinba@users.noreply.github.com>
| 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 |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Except that right now in IDA this specific label style is also used for the underlying parametrization.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
We should try to be consistent
Co-authored-by: Torrin <1843540+torrinba@users.noreply.github.com>
| 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 |
There was a problem hiding this comment.
If they test this, that would be sufficient.
There was a problem hiding this comment.
So all of this got migrated to its own CER omas fetching?
There was a problem hiding this comment.
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.
3 participants
Description
Type of Change
Testing
Tested against imas_composer GA-FDP/imas_composer#94
Pre-Merge Checklist
omas/versionfile0.94.2→0.94.3for bug fixes,0.94.2→0.95.0for new features)Additional Notes