Add simple impurity radiation model (P_imp as a fraction of P_tot_in) #572

theo-brown · 2024-11-26T20:52:57Z

Add a simplified radiation sink model, P_rad = α*(P_aux + P_alpha + P_ohmic)

This is based on the functionality in JETTO:

Todo:

Calculate prad as a fraction of ptot
Add to outputs and plotting
Unit tests
Integration tests

theo-brown · 2024-11-27T09:32:21Z

@Nush395 if you've got time, could you lend a hand with this? I have a couple of questions about implicit sources and sources that link back.

At the moment I'm finding that my implementation is nonzero when I inspect it during runtime but has no effect on the state. In the output, it is 0.

Nush395 · 2024-11-27T10:55:21Z

@Nush395 if you've got time, could you lend a hand with this? I have a couple of questions about implicit sources and sources that link back.

At the moment I'm finding that my implementation is nonzero when I inspect it during runtime but has no effect on the state. In the output, it is 0.

Yes for sure! Happy to answer questions, feel free to drop them on this PR or we can go over on a call if that's easier :)

Nush395 · 2024-11-27T11:06:22Z

@Nush395 if you've got time, could you lend a hand with this? I have a couple of questions about implicit sources and sources that link back.

At the moment I'm finding that my implementation is nonzero when I inspect it during runtime but has no effect on the state. In the output, it is 0.

Just took a quick look at the change. Just a sanity check, have you run the prad config with

'radiation_heat_sink': {'mode': 'model_based'}

?
(As the default mode that you inherit from the RuntimeParams is set to ZERO.)

theo-brown · 2024-11-27T11:13:17Z

Ah, thanks for spotting that. That would be why it wasn't being used! I'll do that and then see where I get to. Sorry for not spotting the obvious thing!

Nush395 · 2024-11-27T11:22:06Z

Ah, thanks for spotting that. That would be why it wasn't being used! I'll do that and then see where I get to. Sorry for not spotting the obvious thing!

No worries at all, I've run into that gotcha myself!

theo-brown · 2024-11-27T12:04:39Z

As of 69f5ada, unit tests fail because I've added a term to post_processed_outputs that is not present in all of the current .nc files. Hence, all test data will need to be regenerated. Is there a speedy way of doing this?

Nush395 · 2024-11-27T12:13:16Z

As of 69f5ada, unit tests fail because I've added a term to post_processed_outputs that is not present in all of the current .nc files. Hence, all test data will need to be regenerated. Is there a speedy way of doing this?

There is, there is a run_and_save_all_benchmarks.py script that we use. See the bottom of https://torax.readthedocs.io/en/latest/contribution_tips.html#contribution-tips that @jcitrin recently added to. (Will also make this a bit more visible, if you have suggestions on where this should be please let us know!)

theo-brown · 2024-11-27T14:37:08Z

@Nush395 it now makes sense to ask my questions!

Should source tests be written on a per-source basis, or use inheritance? See e.g. BootstrapCurrentSourceTest (which is a SourceTestCase whose test_extraction_of_relevant_profile_from_output is identical to that of other psi sources), vs FusionHeatSourceTest (which is an IonElSourceTestCase to avoid reimplementing such functions for all Te + Ti sources).
Where is the OhmicHeatSource tested? As far as I can tell it's the only other source that has links_back=True and I was hunting around to try and copy its test pattern.

Required regenerating some integration tests with new Prad output

Use source_builder in all tests, except from BootstrapCurrentSource and QeiSource

theo-brown · 2024-11-27T15:53:31Z

For the time being I've gone for only inheriting from SourceTestCase, which means:

Inherit
- test_expected_mesh_states
- test_runtime_params_builds_dynamic_params
Reimplement
- test_extraction_of_relevant_profile_from_output
- test_invalid_source_types_raise_errors
- test_source_value

I couldn't quite figure out why test_extraction_of_relevant_profile_from_output and test_invalid_source_types_raise_errors weren't implemented in the parent class - it seems to me that they should be functions of the source properties only? Regardless, this seemed to be the pattern implemented by most other sources, so I went with that.

Nush395 · 2024-11-27T20:34:14Z

@Nush395 it now makes sense to ask my questions!

Hey Theo, sorry for the slow response on this.

Should source tests be written on a per-source basis, or use inheritance? See e.g. BootstrapCurrentSourceTest (which is a SourceTestCase whose test_extraction_of_relevant_profile_from_output is identical to that of other psi sources), vs FusionHeatSourceTest (which is an IonElSourceTestCase to avoid reimplementing such functions for all Te + Ti sources).

In the general case I'd advise inheriting from SourceTestCase as that has some basic test cases relevant to all sources in general. If your source only affects a single profile there is also the SingleProfileSourceTestCase https://github.com/google-deepmind/torax/blob/main/torax/sources/tests/test_lib.py#L113 that has more specialised tests for sources that only affect a single source (as yours does).

Where is the OhmicHeatSource tested? As far as I can tell it's the only other source that has links_back=True and I was hunting around to try and copy its test pattern.

That's a good question. Taking a quick look now it doesn't seem there is one which is something we can rectify. Thanks for raising this.

Nush395 · 2024-11-27T20:39:14Z

I couldn't quite figure out why test_extraction_of_relevant_profile_from_output and test_invalid_source_types_raise_errors weren't implemented in the parent class - it seems to me that they should be functions of the source properties only? Regardless, this seemed to be the pattern implemented by most other sources, so I went with that.

For test_invalid_source_types_raise_errors that is implemented in the SingleProfileSourceTestCase and you are right for the test_extraction_of_relevant_profile_from_output this is likely a legacy thing that could be tidied up and pushed into the parent class now. I can TAL at doing that and doing some test tidying up.

theo-brown · 2024-11-28T08:06:57Z

I can TAL at doing that and doing some test tidying up.

I'm more than happy to tackle this in a separate PR if you want to have a look at the ohmic source? Unless you'd rather do both!

Nush395 · 2024-11-28T08:53:38Z

I can TAL at doing that and doing some test tidying up.

I'm more than happy to tackle this in a separate PR if you want to have a look at the ohmic source? Unless you'd rather do both!

That would be great if you have time, thanks!

jcitrin

Looks really great, thanks!

Appreciate also the extensive testing + extensions to the tests.

torax/examples/prad.py

torax/sources/tests/radiation_heat_sink.py

torax/sources/radiation_heat_sink.py

torax/tests/test_data/test_changing_config_before_BASE_171788.nc

torax/tests/test_data/test_changing_config_before_LOCAL_171788.nc

torax/tests/test_data/test_changing_config_before_REMOTE_171788.nc

torax/tests/test_data/test_implicit.nc

torax/sources/radiation_heat_sink.py

Rename to impurity_radiation_heat_sink Exclude all sinks from the Ptot calculation Use geo.volume_face[-1] rather than recalculating the integral

theo-brown · 2024-12-04T10:01:01Z

Regarding the changed .nc files: these are due to torax/tests/test_run_simulation_main.py.

From what I can tell, tests in this file produce output .nc with all of the post-processed outputs, regardless of whether the corresponding source etc are enabled. I couldn't quite pinpoint why this is the case. It's most likely a bug.

As I've added P_imp to the post-processed outputs, I needed to regenerate these files.

jcitrin · 2024-12-06T15:31:25Z

From what I can tell, tests in this file produce output .nc with all of the post-processed outputs, regardless of whether the corresponding source etc are enabled. I couldn't quite pinpoint why this is the case. It's most likely a bug.

This is a feature not a bug :) . Having zero integrated powers written even when the source isn't included in the simulation was part of the design. The idea was more robust analysis routines and avoiding needing to handle non-existing attributes. However this is not the case for the non-integrated source outputs, so it's a bit inconsistent. We will consider revisiting this in the near future as part of an ongoing output overhaul, and make a single convention.

jcitrin · 2024-12-06T15:38:34Z

torax/tests/test_data/test_iterhybrid_predictor_corrector_impurity_radiation.py

+
+CONFIG = copy.deepcopy(test_iterhybrid_predictor_corrector.CONFIG)
+
+CONFIG['sources']['impurity_radiation_heat_sink'] = {}


nit: while there is a default, in practice users will almost always set the specific fraction. So maybe good to demonstrate that with adding the runtime_param here.

jcitrin · 2024-12-06T15:41:28Z

torax/plotting/configs/default_plot_config.py

@@ -145,8 +145,8 @@
            suppress_zero_values=True,  # Do not plot all-zero data
        ),
        plotruns_lib.PlotProperties(
-            attrs=('q_brems', 'q_rad'),
-            labels=(r'$Q_\mathrm{brems}$', r'$Q_\mathrm{rad}$'),
+            attrs=('q_brems', 'q_imp'),


nit: sorry for being so nitpicky, but while the source name is impurity_radiation_heat_sink , I think it's OK to maintain the shorthand q_rad, P_rad, Q_rad on the user-facing plotting and output level, since it is the standard convention

We'll then have say q_rad, q_brems, q_cycl (coming soon) and this will be clear enough for fusion physicists

Sure, no problem.

jcitrin

Looks good. A couple of small nits. Will bring it in on our end once they're done.

jcitrin · 2024-12-06T16:06:07Z

Great thanks. Bringing it in. That failing test is due to the next JAX release, see #591 . No worries, should be fine in the final merge

jcitrin · 2024-12-09T11:40:52Z

Merged by 0d66813

theo-brown changed the title ~~Add simple radiation model (Prad as a fraction of Ptot)~~ Add simple radiation model (Prad as a fraction of P_tot_in) Nov 26, 2024

theo-brown mentioned this pull request Nov 27, 2024

Feature parity for STEP simulations #420

Open

theo-brown added 4 commits November 27, 2024 15:24

Add radiation_heat_sink, proportional to Qtot

a005f07

Add Prad to post_processing and plots

ff0c854

Required regenerating some integration tests with new Prad output

Support sources with links_back=True

cc16cce

Use source_builder in all tests, except from BootstrapCurrentSource and QeiSource

Add unit test for RadiationHeatSink

7a0936a

theo-brown force-pushed the theo-brown/add-prad branch from 3cd6cad to 7a0936a Compare November 27, 2024 15:35

Merge branch 'main' into theo-brown/add-prad

601cde4

theo-brown marked this pull request as ready for review November 27, 2024 15:41

theo-brown requested a review from jcitrin November 27, 2024 15:48

theo-brown force-pushed the theo-brown/add-prad branch from f737ad0 to 601cde4 Compare November 27, 2024 15:54

Sum over both ion and el heat sources

65e5836

Switch to radially constant fraction of Pin

c6710b6

jcitrin reviewed Nov 29, 2024

View reviewed changes

theo-brown force-pushed the theo-brown/add-prad branch from 6318b87 to fab79cf Compare December 3, 2024 11:16

Minor changes

0d242e4

Rename to impurity_radiation_heat_sink Exclude all sinks from the Ptot calculation Use geo.volume_face[-1] rather than recalculating the integral

theo-brown force-pushed the theo-brown/add-prad branch from fab79cf to 0d242e4 Compare December 3, 2024 11:35

theo-brown changed the title ~~Add simple radiation model (Prad as a fraction of P_tot_in)~~ Add simple impurity radiation model (P_imp as a fraction of P_tot_in) Dec 3, 2024

Add integration tests

576c995

theo-brown force-pushed the theo-brown/add-prad branch from 283730c to 576c995 Compare December 4, 2024 09:53

theo-brown added 2 commits December 4, 2024 10:11

Merge branch 'main' into theo-brown/add-prad

fa9456c

Add static runtime params

0ec0bee

theo-brown force-pushed the theo-brown/add-prad branch 2 times, most recently from 3435c5b to 31c6971 Compare December 4, 2024 11:19

Minor linting fixes

73efa05

theo-brown force-pushed the theo-brown/add-prad branch from 31c6971 to 73efa05 Compare December 4, 2024 11:21

theo-brown requested a review from jcitrin December 4, 2024 11:22

jcitrin reviewed Dec 6, 2024

View reviewed changes

jcitrin approved these changes Dec 6, 2024

View reviewed changes

Change output naming

59d2fa4

copybara-service bot merged commit 0d66813 into main Dec 9, 2024
17 of 20 checks passed

theo-brown deleted the theo-brown/add-prad branch December 12, 2024 07:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add simple impurity radiation model (P_imp as a fraction of P_tot_in) #572

Add simple impurity radiation model (P_imp as a fraction of P_tot_in) #572

theo-brown commented Nov 26, 2024 •

edited

Loading

theo-brown commented Nov 27, 2024

Nush395 commented Nov 27, 2024

Nush395 commented Nov 27, 2024 •

edited

Loading

theo-brown commented Nov 27, 2024

Nush395 commented Nov 27, 2024

theo-brown commented Nov 27, 2024 •

edited

Loading

Nush395 commented Nov 27, 2024

theo-brown commented Nov 27, 2024

theo-brown commented Nov 27, 2024

Nush395 commented Nov 27, 2024 •

edited

Loading

Nush395 commented Nov 27, 2024

theo-brown commented Nov 28, 2024

Nush395 commented Nov 28, 2024

jcitrin left a comment

theo-brown commented Dec 4, 2024

jcitrin commented Dec 6, 2024

jcitrin Dec 6, 2024

jcitrin Dec 6, 2024 •

edited

Loading

theo-brown Dec 6, 2024

jcitrin left a comment

jcitrin commented Dec 6, 2024

jcitrin commented Dec 9, 2024


		CONFIG = copy.deepcopy(test_iterhybrid_predictor_corrector.CONFIG)

		CONFIG['sources']['impurity_radiation_heat_sink'] = {}

Add simple impurity radiation model (P_imp as a fraction of P_tot_in) #572

Add simple impurity radiation model (P_imp as a fraction of P_tot_in) #572

Conversation

theo-brown commented Nov 26, 2024 • edited Loading

theo-brown commented Nov 27, 2024

Nush395 commented Nov 27, 2024

Nush395 commented Nov 27, 2024 • edited Loading

theo-brown commented Nov 27, 2024

Nush395 commented Nov 27, 2024

theo-brown commented Nov 27, 2024 • edited Loading

Nush395 commented Nov 27, 2024

theo-brown commented Nov 27, 2024

theo-brown commented Nov 27, 2024

Nush395 commented Nov 27, 2024 • edited Loading

Nush395 commented Nov 27, 2024

theo-brown commented Nov 28, 2024

Nush395 commented Nov 28, 2024

jcitrin left a comment

Choose a reason for hiding this comment

theo-brown commented Dec 4, 2024

jcitrin commented Dec 6, 2024

jcitrin Dec 6, 2024

Choose a reason for hiding this comment

jcitrin Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

theo-brown Dec 6, 2024

Choose a reason for hiding this comment

jcitrin left a comment

Choose a reason for hiding this comment

jcitrin commented Dec 6, 2024

jcitrin commented Dec 9, 2024

theo-brown commented Nov 26, 2024 •

edited

Loading

Nush395 commented Nov 27, 2024 •

edited

Loading

theo-brown commented Nov 27, 2024 •

edited

Loading

Nush395 commented Nov 27, 2024 •

edited

Loading

jcitrin Dec 6, 2024 •

edited

Loading