Add general-topology support for GOAT-generated geometry - #38
Conversation
|
@hrdoktorx , @WouterDekeyser , @SVandenKerkhof This is my progress towards basing the changes I needed from goat-integration-clean, along with other features, on top of the iterorg branch. This goes along with TME-TFE/goat#10 |
3eae93d to
6e21764
Compare
|
I assume no progress is to be expected here until the Code Camp? |
|
Not sure. Sander recently reviewed my GOAT commit and approved of the changes. But I am fine waiting on the Coad Camp |
|
Well, if you are done with it, don't leave it in draft state. Then it can be looked at and merged much sooner. |
SVandenKerkhof
left a comment
There was a problem hiding this comment.
Hi Jeremy, sorry for the long wait. Looks like all things are there from the goat-integration-clean-rebased branch, plus some additional useful things like the b2mod_mwti changes and core boundary selection. I didn't yet test it out for my cases on my side here, but OK for me to merge and fix potential bugs along the way.
hrdoktorx
left a comment
There was a problem hiding this comment.
It is not clear to me whether this new format for b2fgmtry is backward compatible and/or if the topology data must be provided mandatorily even for standard supported topologies. It looks like, by providing the topological data, the code then assumes a general topology at the exclusion of any of the standard ones. So format 3.2.2 would need to exist in parallel to 3.2.1. That's not how it works. The new format needs to support the old grid types and add new ones, not replace them. Replacing them would destroy the GGD saving of the data into IMAS, so, from the IO point of view, an absolute non-starter.
I also fail to find in this PR any handling of the IMAS write-up.
| ic = ic + 1 | ||
| end do | ||
| end if | ||
| end if |
There was a problem hiding this comment.
In the structured code, for linear cases where there was no topological separatrix, the user could still specify a "separatrix line" with the b2mwti_jsep switch. The same capability should be made available for general topologies where the user can designate a particular flux surface as the reference one (and have its default be the first flux surface, like below, that's be okay, provided it is documented ).
| on_wall = m%fcLbl(iFc).ne.0 | ||
| if (on_wall) then | ||
| if (iFs1.eq.0) wall_face(1) = .true. | ||
| if (iFs1.ne.0 .and. iFs2.eq.0) wall_face(2) = .true. |
There was a problem hiding this comment.
Not sure this change is intentional, or whether it does not break the ITER cases for which it was coded in the first place. Maybe @shtirx or @pshenovaa could try.
| @@ -354,4 +354,5 @@ end module b2mod_version | |||
| c Added srw arrays to b2fplasma | |||
| c Added multi-species netCDF tally output | |||
| c Open-source release | |||
There was a problem hiding this comment.
If introducing a new version number, you need to change the newversion variable at the top of the b2mod_version and provide an accompanying PR at the SOLPS-ITER level with all rebuilt examples with the new version number.
I recommend you read and follow the CONTRIBUTING rules.
| mpg%cvOnClosedSurface = mpg%periodic_bc.eq.1 | ||
| else if (mpg%nnreg(0).eq.2 .or. mpg%nnreg(0).eq.4 .or. | ||
| . mpg%nnreg(0).eq.5 .or. mpg%nnreg(0).eq.8) then | ||
| if (mpg%hasTopologicalData .and. mpg%topoID.eq.0) then |
There was a problem hiding this comment.
This message is gobbledygook. Please write as if the user sees this for the first time.
| mpg%cvOnClosedSurface = mod(mpg%cvReg,4).eq.1 | ||
| else if (mpg%nnreg(0).eq.7) then | ||
| mpg%cvOnClosedSurface = mpg%cvReg.eq.1 | ||
| else |
There was a problem hiding this comment.
I do not think a region of closed surfaces should be assumed. For a linear device, this will always be wrong. For @nthont "coreless DDN" topology as well.
| . 'boundary face has zero ID - inconsistency in ' // | ||
| . 'interconnectivity, check input') | ||
| end if | ||
|
|
There was a problem hiding this comment.
False, incorrect, and misleading.
| @@ -78,6 +78,7 @@ subroutine b2agdr (ninp, nout, g, m) | |||
| integer idum(0:9), redef_gmtry | |||
| integer artificial_slab | |||
| character lblgm*120 | |||
There was a problem hiding this comment.
That's not how this works. You ALWAYS write the output files with "newversion. You can choose to use b2fgmtry_version` here, which is equivalent.
| idum(2) = -1 | ||
| endif | ||
| call cfwuin (nout(1), 3, idum, 'nx,ny,nncut') | ||
|
|
There was a problem hiding this comment.
Looks to me that this is not backward compatible. Does this mean that version 03.002.002 can only be used with general topologies and not the standard ones?
| else | ||
| call xerrab('Grid version not equal to 03.002.00') | ||
| endif ! if of the grid_version | ||
| else |
There was a problem hiding this comment.
Again, lead the user to run b2us to convert any older b2fgmtry format.
| @@ -529,10 +529,10 @@ subroutine b2agmt (g, m, isymm, artificial_slab) | |||
| ! With this method, numerical error on the 5-point contribution on | |||
| ! gradient calculation is significantly reduced for general grids | |||
|
|
|||
There was a problem hiding this comment.
I still need a justification, with a concrete example, for the necessity to keep fcQgam positive.
|
I understand what you're trying to do, but there are a great many things wrong with your approach, and it is incomplete in IO-unacceptable ways. Please read and address my comments in detail. |
This PR adds support for optional explicit topology metadata in
b2fgmtry, for GOAT-generated geometries and other general-topology grids.The main changes are:
b2fgmtryheader reads across B2.5 entry points.b2fgmtryversion03.002.002topology records.GEOMETRY_GENERAL.b2.transport.inputfileprofiles on general topologies.The file format history is updated in
b2mod_version.Ffor the new optional topology records.Dependency
This branch is stacked on the B2.5 bugfix PR #17 . Please review/merge that PR first, then this branch can be rebased or retargeted cleanly onto
feature/wg-release.