Skip to content

[WIP] add radiation momentum deposition in photoionization#2007

Draft
BenWibking wants to merge 2 commits into
developmentfrom
wip/photoionization-vc-work-term
Draft

[WIP] add radiation momentum deposition in photoionization#2007
BenWibking wants to merge 2 commits into
developmentfrom
wip/photoionization-vc-work-term

Conversation

@BenWibking

@BenWibking BenWibking commented Jun 27, 2026

Copy link
Copy Markdown
Collaborator

Description

Please write a short paragraph describing what changes this pull request makes to the code. It should provide sufficient context so that a reviewer can efficiently browse the source code changes.

Related issues

Fixes #1988.

Checklist

Before this pull request can be reviewed, all of these tasks should be completed. Denote completed tasks with an x inside the square brackets [ ] in the Markdown source below:

  • I have added a description (see above).
  • I have added a link to any related issues (if applicable; see above).
  • I have read the Contributing Guide.
  • I have added tests for any new physics that this PR adds to the code.
  • (For quokka-astro org members) I have manually triggered the GPU tests with the magic comment /azp run.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a new test case (DTypeFrontVC) and implements the O(v/c) radiation-pressure work term in the photochemistry solver to deposit absorbed photon momentum into the gas. Feedback on these changes highlights two issues: first, a critical bug in photochemistry.hpp where the momentum update is not scaled by energy_update_factor during multi-stage integration (while the kinetic energy subtraction is), which violates energy conservation; second, a potential division-by-zero in the test file if no chem-band flux is absorbed.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/radiation/photochemistry.hpp Outdated
Comment on lines +220 to +222
const amrex::Real rel_err = std::abs(check.gas_px - check.expected_px) / check.expected_px;
const amrex::Real transverse_momentum = std::sqrt(check.gas_py * check.gas_py + check.gas_pz * check.gas_pz);
const amrex::Real transverse_rel = transverse_momentum / check.expected_px;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If no chem-band flux is absorbed (e.g., due to a setup failure or bug), check.expected_px will be 0.0_rt. This will cause a division-by-zero when computing rel_err and transverse_rel, resulting in NaN or inf values. It is safer to guard these divisions by checking if check.expected_px > 0.0_rt.

	const amrex::Real rel_err = (check.expected_px > 0.0_rt) ? (std::abs(check.gas_px - check.expected_px) / check.expected_px) : std::numeric_limits<amrex::Real>::infinity();
	const amrex::Real transverse_momentum = std::sqrt(check.gas_py * check.gas_py + check.gas_pz * check.gas_pz);
	const amrex::Real transverse_rel = (check.expected_px > 0.0_rt) ? (transverse_momentum / check.expected_px) : std::numeric_limits<amrex::Real>::infinity();

@BenWibking

Copy link
Copy Markdown
Collaborator Author

/azp run quick

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add v/c work term in photoionization

1 participant