Using the primitive variable index classes in more places (+ minor chores) by pcarruscag · Pull Request #1476 · su2code/SU2

pcarruscag · 2021-12-24T16:34:26Z

Cleanup related with #1392

…into solver_index_cleanup

pcarruscag · 2021-12-25T18:39:33Z

+  /*--- Repeat as many times as necessary to handle disconnected graphs. ---*/
+  while (Result.size() < nPointDomain) {


Reason for the residual changes.

Ok: RCM in practice reorders points/point-ID's such that neighboring points have similar ID's which itself is beneficial for stencil as data for neighboring points is close together in memory which is a good thing for minimizing cache misses.

Now, why would one exclude halo points from the re-ordering? If the reordering is done per process it shouldn't really matter from a naive standpoint ... or it should even be worse considering that the halo point values are not ordered with the rest?

Enlighten me if that can be briefly explained (answer given in Dev-meeting)

-> (from dev meeting) Halo points should be at the end of the point list as a convention.
The disconnected domain parts were the change (as they happen in the CHT pin case where disconnected pin-pieces are treated as 1 zone)

WallyMaier · 2021-12-29T18:30:46Z

-        if (FlowRegime == ENUM_REGIME::COMPRESSIBLE) Grad_Temp[iDim] = nodes->GetGradient_Primitive(iPoint, 0, iDim);
-
-        if (FlowRegime == ENUM_REGIME::INCOMPRESSIBLE) Grad_Temp[iDim] = nodes->GetGradient_Primitive(iPoint, nDim + 1, iDim);
+        Grad_Temp[iDim] = nodes->GetGradient_Primitive(iPoint, prim_idx.Temperature(), iDim);


WallyMaier

LGTM! Thanks!

TobiKattmann

💐 many good cleanups where hardcoded indexes were used. And good that CONV_CRITERIA is now deprecated

TobiKattmann · 2022-01-05T14:28:36Z

+          else if (!option_name.compare("CONV_CRITERIA"))
+            newString.append("CONV_CRITERIA is deprecated. SU2 will choose the criteria automatically based on the CONV_FIELD.\n"
+                             "RESIDUAL for any RMS_* BGS_* value. CAUCHY for coefficients like DRAG etc.\n\n");


Finally 💪 This will definitely hit a few people. I saw quite a few cases where the warning was consequently ignored :) ... I also did bother to change it in some instances (see below 🙈 )

TobiKattmann · 2022-01-05T14:53:15Z

+  /*--- Repeat as many times as necessary to handle disconnected graphs. ---*/
+  while (Result.size() < nPointDomain) {


Ok: RCM in practice reorders points/point-ID's such that neighboring points have similar ID's which itself is beneficial for stencil as data for neighboring points is close together in memory which is a good thing for minimizing cache misses.

Now, why would one exclude halo points from the re-ordering? If the reordering is done per process it shouldn't really matter from a naive standpoint ... or it should even be worse considering that the halo point values are not ordered with the rest?

Enlighten me if that can be briefly explained (answer given in Dev-meeting)

TobiKattmann · 2022-01-05T14:58:11Z

+      /*--- Move the start of the queue, equivalent to taking from the front of
+       * the queue and inserting at the end of the result. ---*/
+      AddPoint = Result[QueueStart];
+      ++QueueStart;

-    AuxQueue.clear();
-    for (auto iNode = 0u; iNode < nodes->GetnPoint(AddPoint); iNode++) {
-      auto AdjPoint = nodes->GetPoint(AddPoint, iNode);
-      if ((!inQueue[AdjPoint]) && (AdjPoint < nPointDomain)) {
-        AuxQueue.push_back(AdjPoint);
+      /*--- Add all adjacent nodes to the queue in increasing order of their
+       degree, checking if the element is already in the queue. ---*/


inconsistent commenting style (* used/not-used in the 2nd line comment)
Verdict: U n a c c e p t a b l e

TobiKattmann · 2022-01-05T14:59:34Z

+  for (const auto status : InQueue) {
+    if (!status) SU2_MPI::Error("RCM ordering failed", CURRENT_FUNCTION);


👍 for adding a check

TobiKattmann · 2022-01-05T17:19:21Z

+  /*--- Repeat as many times as necessary to handle disconnected graphs. ---*/
+  while (Result.size() < nPointDomain) {


-> (from dev meeting) Halo points should be at the end of the point list as a convention.
The disconnected domain parts were the change (as they happen in the CHT pin case where disconnected pin-pieces are treated as 1 zone)

TobiKattmann · 2022-01-05T17:25:45Z

 %
 % --------------------------- CONVERGENCE PARAMETERS --------------------------%
 %
-CONV_CRITERIA= RESIDUAL


Wasn't me - Shaggy, 2000

TobiKattmann · 2022-01-07T08:28:55Z

    inline IndexType CvTotal() const { return nDim+8; }
+
+    /*--- For compatible interface with NEMO. ---*/
+    inline IndexType Temperature_ve() const { return std::numeric_limits<IndexType>::max(); }


I need to

#include <limits>

for this to work using gcc/11.1.0 + openmpi/4.1.2. as well as 11.2.0 + 4.0.5. The test-builds pass(ed) so it cannot be a major issue.
This stackoverflow post gives resource that the limits header is used much less widely in gcc11 standard library headers -> requiring to explicitly include it.

And I guess we want to assure foward-compatibility ;)

use the variable index classes in more places

58833bb

pcarruscag added the changelog:chore label Dec 24, 2021

pr-triage Bot added the PR: unreviewed label Dec 24, 2021

pcarruscag and others added 4 commits December 24, 2021 16:34

Merge branch 'develop' into solver_index_cleanup

9ca86e9

fix

6ae3b2a

Merge branch 'solver_index_cleanup' of https://github.com/su2code/SU2 …

dfc9e54

…into solver_index_cleanup

fix comments

377562e

pcarruscag changed the title ~~Using the primitive variable index classes in more places~~ Using the primitive variable index classes in more places (+ minor chores) Dec 25, 2021

pcarruscag added 4 commits December 25, 2021 17:30

deprecate CONV_CRITERIA

ab6e2b2

update periodic pin cases after fix to RCM

f36c376

throw error if rotational periodicity is used with NEMO

41624d6

CONV_CRITERIA

5d687fa

pcarruscag commented Dec 25, 2021

View reviewed changes

fix #1477

bc5245b

pcarruscag linked an issue Dec 26, 2021 that may be closed by this pull request

The NS continuous adjoint solver tries to load a restart even with RESTART_SOL = NO #1477

Closed

delete a load of code that didnt work

fb59ee7

WallyMaier reviewed Dec 29, 2021

View reviewed changes

pcarruscag merged commit 9efe761 into develop Dec 31, 2021

pcarruscag deleted the solver_index_cleanup branch December 31, 2021 16:02

pr-triage Bot added PR: merged and removed PR: unreviewed labels Dec 31, 2021

TobiKattmann reviewed Jan 5, 2022

View reviewed changes

pr-triage Bot added PR: unreviewed and removed PR: merged labels Jan 5, 2022

TobiKattmann reviewed Jan 7, 2022

View reviewed changes

Navigation Menu

Uh oh!

Using the primitive variable index classes in more places (+ minor chores) - #1476

Using the primitive variable index classes in more places (+ minor chores)#1476
pcarruscag merged 11 commits into
developfrom
solver_index_cleanup

pcarruscag commented Dec 24, 2021

Uh oh!

pcarruscag Dec 25, 2021

Uh oh!

TobiKattmann Jan 5, 2022 •

edited

Loading

Uh oh!

TobiKattmann Jan 5, 2022

Uh oh!

WallyMaier Dec 29, 2021

Uh oh!

WallyMaier left a comment

Uh oh!

TobiKattmann left a comment

Uh oh!

TobiKattmann Jan 5, 2022 •

edited

Loading

Uh oh!

TobiKattmann Jan 5, 2022 •

edited

Loading

Uh oh!

TobiKattmann Jan 5, 2022

Uh oh!

TobiKattmann Jan 5, 2022

Uh oh!

TobiKattmann Jan 5, 2022

Uh oh!

TobiKattmann Jan 5, 2022

Uh oh!

TobiKattmann Jan 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		/--- Repeat as many times as necessary to handle disconnected graphs. ---/
		while (Result.size() < nPointDomain) {

		for (const auto status : InQueue) {
		if (!status) SU2_MPI::Error("RCM ordering failed", CURRENT_FUNCTION);

Uh oh!

Conversation

pcarruscag commented Dec 24, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobiKattmann Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WallyMaier left a comment

Choose a reason for hiding this comment

Uh oh!

TobiKattmann left a comment

Choose a reason for hiding this comment

Uh oh!

TobiKattmann Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TobiKattmann Jan 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

TobiKattmann Jan 5, 2022 •

edited

Loading

TobiKattmann Jan 5, 2022 •

edited

Loading

TobiKattmann Jan 5, 2022 •

edited

Loading