solved IMAS plasma composition surprising behaviour #1811
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix IMAS plasma composition surprising behaviour (#1789)
Problem
When "He" was added as a new constants.IonProperty (alias to He4), it started being picked up as an impurity from IMAS IDS data, significantly modifying the step_flattop_bgb result (~50% different temperatures). The root cause was that the code silently ignored impurities not in constants.ION_PROPERTIES_DICT.keys(), leading to unexpected behavior when new ions were added to the dictionary.
Solution
This PR addresses the issue by:
Replacing expected_impurities with excluded_impurities: Changed the API to be more intuitive - users can now explicitly exclude impurities they don't want to process, rather than having to specify what they expect.
Adding error handling for unidentified impurities: The function now raises a clear ValueError when an ion is found in the IDS that is not in constants.ION_PROPERTIES_DICT and not in excluded_impurities. This prevents silent failures and makes it explicit when new ions are being processed.
Improved user control: Users can now explicitly control which impurities are processed using the excluded_impurities argument, preventing surprises when new ions are added to ION_PROPERTIES_DICT.
Changes Made
Modified plasma_composition_from_IMAS() in torax/_src/imas_tools/input/core_profiles.py:
Replaced expected_impurities parameter with excluded_impurities
Added validation to raise ValueError for unidentified impurities
Added logic to skip excluded impurities during processing
Updated all tests to use the new excluded_impurities API
Updated documentation strings to reflect the new behavior
Testing
Updated existing tests in torax/_src/imas_tools/input/tests/core_profiles_test.py
Added tests for the new error handling behavior
All linting checks pass
Impact
This change ensures that:
Users are explicitly notified when unidentified impurities are found
Users have control over which impurities are processed
No more silent failures that lead to unexpected simulation results
The API is more intuitive and user-friendly
Fixes #1789