Skip to content

Minimal support for cases with more than 2 X-points - #41

Open
hrdoktorx wants to merge 7 commits into
feature/wg-releasefrom
feature/more-than-2-Xpts
Open

Minimal support for cases with more than 2 X-points#41
hrdoktorx wants to merge 7 commits into
feature/wg-releasefrom
feature/more-than-2-Xpts

Conversation

@hrdoktorx

@hrdoktorx hrdoktorx commented Aug 24, 2026

Copy link
Copy Markdown
Collaborator

The mapping data structure now stores the indices of the active and inactive X-points, and the code uses them instead of hard-coded indices 1 and 2. The active X-point is the first one to be found that is a vertex to a cell inside volumetric region 1 (the core). The inactive X-pt is chosen to be the X-point closest in Psi to the active X-point. If they do not lie on the same flux surface, the difference between DDN_TOP and DDN_BOTTOM topologies is simply whether the X-point is above or below the z=0 plane.

@hrdoktorx
hrdoktorx requested a review from jlore August 24, 2026 19:21
@jlore

jlore commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Added some comments.

Note mainly for me: Merging this will require some adaptation of my general topology PR, which I will handle after it gets reviewed by the Leuven team.

@hrdoktorx

Copy link
Copy Markdown
Collaborator Author

@jlore Sorry, I do not see your comments.

@jlore

jlore commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

Ok, I tried commenting on the in-line code since I don't have this branch checked out. I'll try to @ you there to see if they are visible

@hrdoktorx

Copy link
Copy Markdown
Collaborator Author

I still see nothing.

Comment thread src/ids/b2mod_geometry.F90 Outdated
Comment thread src/ids/b2mod_geometry.F90
Comment thread src/ids/b2mod_geometry.F90
Comment thread src/ids/b2mod_geometry.F90 Outdated
Comment thread src/modules/b2us_geo.F
Comment thread src/modules/b2us_map.F Outdated
@jlore

jlore commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator

I had to "submit review"

…purious ones + bug fixes suggested by Jeremy Lore
@hrdoktorx

Copy link
Copy Markdown
Collaborator Author

I would appreciate from the reviewers (@antonellozito @SVandenKerkhof @WouterDekeyser @SjoerdPR ) at least the courtesy of replying to my PRs with no more than an order of magnitude slower than I respond to theirs.
@jlore I still await your explanation for your objection to line 1067 in b2us_geo.

@jlore

jlore commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Missed that, and you are correct in your comment. My comment on 1067 can be ignored

@antonellozito

Copy link
Copy Markdown
Collaborator

Sorry, indeed I did not give the official OK in the PR even though I had given my informal OK in the Slack channel... apologies for that.
The changes are fine for me. As I said, these changes would be obsolete in case it is the external gridding program which already tells to B2.5 which is the topology, like I am doing right now in my forked branches which rely on GOAT, but whether/how to use this approach in the future will be widely discussed at the Code Camp. For the time being, this is fine for the official branch.

@WouterDekeyser

Copy link
Copy Markdown
Collaborator

Also agree with current changes - pending outcome of discussions on implementation of more generalized topologies.

@SjoerdPR

SjoerdPR commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

I was asked to check if a specific case (that used to segfault before manually fixing face regions) segfaulted with this branch.

With the regular wg-release (after fixing face regions in my case), the segfault no longer happens. However, with this branch, the following error comes up (debug print statesments added right above

B2.5/src/modules/b2us_geo.F

Lines 1249 to 1250 in c550bf3

call xertst (match_found .or. mpg%nXpt.eq.0,
& 'No matching face found for strike point !')
).

match_found= T
nXpt= 6
match_found= F
nXpt= 6
*** XERRAB: program will stop. ***
No matching face found for strike point !
Call chain follows.
b2mndr_0
b2mn
STOP xerrab--error

I am not planning to continue with this case for my work, and it seems like a 50/50 whether this is an error with the PR or an error in my case that simply interacts poorly with the changes made in the PR. To avoid wasting time on the latter, I think the most efficient way to proceed would be to test the changes in this PR with a wide-grid case that is known to be well set-up, rather than my case.

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.

5 participants