Map ICRH minority species to plasma composition #1810
Conversation
This commit addresses issue google-deepmind#529 by integrating ICRH minority species with the plasma composition system, ensuring physical consistency across all plasma calculations. Changes: - Add optional 'minority_species' parameter to IonCyclotronSourceConfig that allows specifying which species in plasma_composition to use as the ICRH minority (e.g., 'He3', 'D', 'T') - Add _get_minority_concentration_from_composition() helper function that extracts minority concentration from plasma composition, supporting: * Minority species in main ions (for hydrogenic species) * Minority species in impurities (for helium species) * All three impurity modes: fractions, n_e_ratios, n_e_ratios_Z_eff - Update icrh_model_func() to read minority concentration from plasma_composition when minority_species is set - Maintain backward compatibility: existing configs using minority_concentration parameter continue to work Testing: - Add test_source_with_minority_species_from_composition() to verify new functionality with He3 minority in impurities - All existing ICRH tests pass (backward compatibility confirmed) - All 651 source tests pass (no regressions) This implementation completes the first task in issue google-deepmind#529: "Map ICRH minority to either main-ion (if hydrogenic) or impurity (if He)"
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
I've signed the CLA for this github account before but re-signed it for the e-mail used on this commit. Not sure how to retrigger the CLA check after re-signing. |
|
Manually triggered it for you, CLA check pass fine now! |
jcitrin
left a comment
There was a problem hiding this comment.
Thanks for this contribution, and sorry that it fell between the cracks leading to a delayed review!
| minority_concentration_profile, geo | ||
| ) | ||
| else: | ||
| # Use legacy parameter (backward compatibility) |
There was a problem hiding this comment.
add a TODO to remove the backwards incompatibility in V2
| minority_concentration: torax_pydantic.TimeVaryingScalar = ( | ||
| torax_pydantic.ValidatedDefault(0.03) | ||
| ) | ||
| minority_species: Annotated[str | None, torax_pydantic.JAX_STATIC] = None |
There was a problem hiding this comment.
At the moment, the TORIC-NN being used is only valid for He3 minority. A Pydantic validation can assure that only He3 is a valid input.
| elif minority_species in plasma_comp.impurity_names: | ||
| # For impurities, need to get the species-specific density | ||
| # The impurity concentration depends on the impurity mode | ||
| if isinstance( |
There was a problem hiding this comment.
This internal branching adversely impacts readability. Maybe use a match case pattern for the 3 cases?
| ) | ||
|
|
||
| else: | ||
| raise ValueError( |
There was a problem hiding this comment.
could check for this in the beginning of the function, and avoid this extra else
| else: | ||
| raise ValueError( | ||
| f'Minority species {minority_species} not found in' | ||
| ' impurity_fractions.' | ||
| ) |
There was a problem hiding this comment.
can remove this if checking for minority species not being in main_ions or impurities in the beginning of the function
| else: | ||
| raise ValueError( | ||
| f'Minority species {minority_species} not found in' | ||
| ' impurity_fractions.' | ||
| ) |
There was a problem hiding this comment.
can remove this if checking for minority species not being in main_ions or impurities in the beginning of the function
| species_fraction = core_profiles.impurity_fractions[ | ||
| minority_species | ||
| ] | ||
| return total_impurity_concentration * species_fraction |
There was a problem hiding this comment.
this does not provide the true ne_ratio of the impurity, due to . We also need to multiply by the impurity_density_scaling.
See here:
With the explanation why here:
| core_profiles.n_impurity.value / core_profiles.n_e.value | ||
| ) | ||
| species_fraction = core_profiles.impurity_fractions[minority_species] | ||
| return total_impurity_concentration * species_fraction |
There was a problem hiding this comment.
as above, this does not provide the true ne_ratio of the impurity, due to . We also need to multiply by the impurity_density_scaling.
|
No further action needed from your side. The comments are minor and we can take care of them in internal review. Thanks again for this contribution |
This commit addresses issue #529 by integrating ICRH minority species with the plasma composition system, ensuring physical consistency across all plasma calculations.
Changes:
Testing:
This implementation completes the first task in issue #529: "Map ICRH minority to either main-ion (if hydrogenic) or impurity (if He)"