Skip to content

Preserve face orientation in intflux - #49

Merged
hrdoktorx merged 2 commits into
iterorganization:feature/wg-releasefrom
jlore:bugfix/intflux-face-orientation
Sep 9, 2026
Merged

Preserve face orientation in intflux#49
hrdoktorx merged 2 commits into
iterorganization:feature/wg-releasefrom
jlore:bugfix/intflux-face-orientation

Conversation

@jlore

@jlore jlore commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Corrects the reconstruction of cell-centered fluxes in intflux by preserving the sign of the face projection factor fcQalf.

intflux derives an effective face velocity by dividing the signed face flux by the interpolated density and projected face area:

velocity = faceflux / (density * fcS * fcQalf)

The previous implementation used abs(fcQalf). On WG grids, face orientation can reverse across topology cuts, causing fcQalf to change sign. Taking its absolute value discarded that orientation information and could produce an incorrect sign in the reconstructed velocity and interpolated cell-centered flux.

Using the signed value of fcQalf ensures that equivalent physical flows retain a consistent direction across topology cuts. This also matches the signed-area convention already used by divide_by_area in b2us_geo.F

@jlore
jlore requested a review from hrdoktorx September 8, 2026 12:31
@jlore
jlore marked this pull request as ready for review September 8, 2026 12:33
@hrdoktorx

Copy link
Copy Markdown
Collaborator

I do not review draft PRs. But you can encourage me by providing a detailed explanation of why this change is required.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

AD-generated intflux_* implementations in src/differentiation/** still use abs(fcQalf) and will be inconsistent with the updated forward model until regenerated/updated.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates the face-to-cell flux interpolation in intflux to preserve face-orientation sign (via fcQalf) when computing effective velocities, with accompanying in-code rationale.

Changes:

  • Clarified the algorithm comment to explain why face orientation sign must be preserved across topology cuts.
  • Changed effective-velocity computation to divide by the signed projected face area geo%fcS * geo%fcQalf (removing abs(...)).
File summaries
File Description
src/utility/intflux.F Preserve face orientation sign in effective-velocity calculation and document the rationale.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/utility/intflux.F
Comment thread src/utility/intflux.F

@hrdoktorx hrdoktorx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@copilot has requested changes. I agree with them.

Use the signed fcQalf projection in the primal and differentiated intflux routines so face orientation is retained across topology cuts.
@jlore
jlore force-pushed the bugfix/intflux-face-orientation branch from 791d485 to d6a25f5 Compare September 8, 2026 12:52
@hrdoktorx
hrdoktorx dismissed their stale review September 8, 2026 12:55

Requested changes applied, thank you. Still need to see an example of why this is needed.

@jlore

jlore commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator Author

Ok, should be ready now.

@hrdoktorx

Copy link
Copy Markdown
Collaborator

I'll wait for the KUL review, since you ignored my request for a documented example of where this change matters.

@jlore

jlore commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Left corrected, right original

image

@jlore

jlore commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

ITER case: Right original, left corrected
image

@jlore

jlore commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

@hrdoktorx Here are some examples, the 2nd one is an ITER case

@hrdoktorx

Copy link
Copy Markdown
Collaborator

It does look ugly. Walk me through them, please.

@hrdoktorx

Copy link
Copy Markdown
Collaborator

@pshenovaa @shtirx Do you also see such things in your ITER runs?

@jlore

jlore commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

I added a description to the PR, but here is another description:

intflux reconstructs a face velocity as
u_face = faceflux / (density_face * fcS * fcQalf)
Across a topology cut, the face orientation can reverse, so fcQalf changes sign. The old code used abs(fcQalf), which discarded that orientation and gave the reconstructed velocity the wrong sign on those faces. When those face values were averaged to cell centers, the error appeared as the discontinuity visible along the topology cut in the “before” plot.
The corrected code retains the sign of fcQalf, so the orientation in the stored face flux is properly canceled and the field remains continuous in the “after” plot.

@jlore

jlore commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

And the ITER case I am plotting is from Andrei

@jlore

jlore commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Note this is just a b2plot error, it's already handled correctly elsewhere as I noted in the description

@hrdoktorx

Copy link
Copy Markdown
Collaborator

Is intflux only called by b2plot?

@jlore

jlore commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Yes, from what I can see. Only in b2plot and caltraj, which is only used by b2plot.

@hrdoktorx hrdoktorx left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Since it is only a b2plot error, I am satisfied. Trust, but verify.

@hrdoktorx
hrdoktorx merged commit 4afb03f into iterorganization:feature/wg-release Sep 9, 2026
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.

3 participants